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

Implementing new methods #30

Merged
merged 7 commits into from
Jun 28, 2024
Merged

Conversation

ElySeraidarian
Copy link
Collaborator

This PR aims to solve #23, everything should be working.

@ElySeraidarian ElySeraidarian requested a review from RiboRings June 26, 2024 09:20
@RiboRings
Copy link
Member

Thank you for the PR!

What does .cacheCommonInfo do? Is it something essential, or can we avoid using it?

@ElySeraidarian
Copy link
Collaborator Author

ElySeraidarian commented Jun 26, 2024

It is supposed to keep in cache data that we would need through our panels so that we don't have to compute them each time (for example we could use .getCachedCommonInfo to get colData information instead of calling colData function for each field in inputs). I'd have to replace these in order to be useful, otherwise it's pretty useless. (also if functions are called only once shouldn't be useful)

@RiboRings
Copy link
Member

Ok nice!

Do you see any place that we are using a function multiple times and it would be beneficial to use .getCachedCommonInfo instead?

We should aim for minimal code, so if it is not strictly necessary we can avoid using this extra method.

@ElySeraidarian
Copy link
Collaborator Author

Not really, maybe in RowTreePlot ? Anyways I don't think this is really useful but maybe in the future it can be?

@RiboRings
Copy link
Member

Then we can skip it. How about .refineParameters?

@ElySeraidarian
Copy link
Collaborator Author

This is linked to .cacheCommonInfo method (they go together), this is to be sure data is well loaded, especially for slots we are using in inputs ( making sure there is no NA value or invalid value). If we remove .cacheCommonInfo, we can also remove this one also as it uses the cache data (and I don't think it is really useful too as it was perfectly working before)

@RiboRings
Copy link
Member

Ok thanks for digging in those methods!

Then we can remove those and just keep .exportOutput. It would be great if we can add tests for .exportOutput for the different panels.

@ElySeraidarian
Copy link
Collaborator Author

Made changes, not really familiar with tests but I tried my best with what already existed & iSEE tests

@RiboRings RiboRings merged commit 3b25e4a into microbiome:devel Jun 28, 2024
3 of 4 checks passed
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.

2 participants