Bugfix/estelle/61/num score sets and experiments#447
Bugfix/estelle/61/num score sets and experiments#447EstelleDa merged 19 commits intorelease-2025.2.0from
Conversation
Release 2025.1.2
filter condition so that it works correctly.
…02-01 and will be fully unsupported by 2025-04-15. Workflows using the ubuntu-20.04 image label should be updated to ubuntu-latest, ubuntu-22.04, ubuntu-24.04.
…appedVariants Modify the mapped variants' filter condition so that it works correctly.
…t and score set in router functions. Found a bug in add_contributor function that leads to some tests can't pass. Fix that one first then I'll keep adding more tests.
bencap
left a comment
bencap
left a comment
There was a problem hiding this comment.
Thanks for working on this Estelle. The calculation logic looks good, and I left a few minor comments. I messaged you on Slack about how we might fix the failing test cases.
In the future, I want to think about how to integrate some of these calculations into our Pydantic classes. It would be nice if we could just create an Experiment view model and have these fields be calculated automatically. Because of the user permissions, I think it is a bit tricky though and I want to wait until we are on Pydantic 2.0+ at least to try it.
src/mavedb/lib/experiments.py
Outdated
| filtered_score_sets = [score_set for score_set in filter_superseded_score_set_tails if score_set is not None] | ||
| filtered_score_set_urns = [] | ||
| if filtered_score_sets: | ||
| filtered_score_set_urns = list(set([score_set.urn for score_set in filtered_score_sets])) | ||
| filtered_score_set_urns.sort() |
There was a problem hiding this comment.
I believe this can be done in one line
| filtered_score_sets = [score_set for score_set in filter_superseded_score_set_tails if score_set is not None] | |
| filtered_score_set_urns = [] | |
| if filtered_score_sets: | |
| filtered_score_set_urns = list(set([score_set.urn for score_set in filtered_score_sets])) | |
| filtered_score_set_urns.sort() | |
| filtered_score_sets = list(set([score_set for score_set in filter_superseded_score_set_tails if score_set is not None])).sort() |
There was a problem hiding this comment.
I'll modify this part. We need the urn list.
| expected_response = update_expected_response_for_created_resources( | ||
| deepcopy(TEST_MINIMAL_SEQ_SCORESET_RESPONSE), experiment, response_data | ||
| ) | ||
| expected_response["experiment"].update({"numScoreSets": 1}) |
There was a problem hiding this comment.
Would it make more sense to add a property to the expected response constant in tests.helpers.constants with this property so that we don't have to update the expected response each time?
There was a problem hiding this comment.
I considered this one before. It's a dynamic value, especially when testing the permission, whether private or public, for contributors and superseding cases. That's why I finally don't put it in tests.helpers.constants. Even though we put it in tests.helpers.constants, most of the time we still need to update it manually. I modify this part to set it to 0 in tests.helpers.constants, and manually update it on the rest tests. I don't have any other better solution for this part.
There was a problem hiding this comment.
Yeah, that's true. I guess it can probably work like the keys after the # keys to be set after receiving response comment. In the default case the key is 0, but we have to manually set it when we update. I like what you changed it to.
There was a problem hiding this comment.
I moved it to the keys part.
I agree with you. I also tried to process the data in Pydantic classes, but finally I gave up because we need function calls. We can try it when we are on Pydantic 2.0+ and see whether we can handle the permission check problem. Otherwise, it's really a messy task if we need to modify them. |
2 participants
Due to the superseding score set and reading permission checks, it's a little complex than expected.