Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always request all study vocabs #342

Merged
merged 12 commits into from
Jan 5, 2024
Merged

Always request all study vocabs #342

merged 12 commits into from
Jan 5, 2024

Conversation

d-callan
Copy link
Contributor

@d-callan d-callan commented Dec 20, 2023

this, together w VEuPathDB/veupathUtils#32, should fix the issue w the histo seen in VEuPathDB/veupathUtils#31

depends on VEuPathDB/EdaCommon#49
depends on VEuPathDB/plot.data#237

needs testing yet

@d-callan d-callan requested a review from bobular December 20, 2023 16:56
Copy link
Member

@bobular bobular left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from the couple of typos I think I spotted.

I didn't understand what the DynamicDataSpecImpl -> DynamicDataSpec changes were about. Just renaming for style reasons?

@d-callan
Copy link
Contributor Author

d-callan commented Jan 3, 2024

now tested w boxplots, cleaned up some, ready for final review

Copy link
Member

@ryanrdoherty ryanrdoherty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't say I understand a lot of the interaction between the collection metadata and the R function calls. It occurs to me that we have an awful lot of R API stuff in here that could maybe be factored into some sort of RPlotClient class, instantiated with the default subset or something? Would be nice to limit the stuff in AbstractPlugin to just providing an API to get the required data and deliver the generated results back to the web client.

);
new StreamSpec(DEFAULT_SINGLE_STREAM_NAME, outputEntityId)
.addVars(plotVariableSpecs)
// TODO can we make this automagical?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear to me what you want to be automagical? We probably can with some inheritance if you want to generalize some things. Just hit me up on slack if you want to discuss. Looks like there might be a lot of repetition/cut-and-paste in some of these plugins which we would like to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a lot of repetition, and i hate it, and every time i think about improving the situation it seems to turn into a can of worms.. i start thinking things like 'well in an ideal world id actually be doing some entirely other thing' and things spiral out of control. so if you have ideas, id be open to hearing them

@d-callan
Copy link
Contributor Author

d-callan commented Jan 5, 2024

Can't say I understand a lot of the interaction between the collection metadata and the R function calls

me neither 😸

happy to move stuff out of AbstractPlugin. i was also thinking some of that probably is better off in edacommon

@d-callan d-callan merged commit 4e67873 into master Jan 5, 2024
@d-callan d-callan deleted the all-study-vocabs branch January 5, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants