Skip to content

Conversation

@dpopleton
Copy link
Contributor

Both of these new values are properly updated in the assembly_sequence updater function. Full test coverage of all reasonable cases is provided in core_1
✨ ✅

Both of these new values are properly updated in the assembly_sequence updater function. Full test coverrage of all reasonable cases is provided in core_1
@dpopleton dpopleton requested a review from marcoooo December 5, 2023 01:11
@marcoooo marcoooo requested a review from ens-st3 December 5, 2023 14:00
@dpopleton dpopleton requested a review from marcoooo December 5, 2023 15:45
Copy link
Contributor

@marcoooo marcoooo left a comment

Choose a reason for hiding this comment

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

there is no data in tests related to this feature. Shouldn't you add some?

@dpopleton
Copy link
Contributor Author

there is no data in tests related to this feature. Shouldn't you add some?

Please check out the changes in the tests. There is full coverage for this feature.

@dpopleton dpopleton requested a review from marcoooo December 5, 2023 16:02
@marcoooo
Copy link
Contributor

marcoooo commented Dec 5, 2023

there is no data in tests related to this feature. Shouldn't you add some?

Please check out the changes in the tests. There is full coverage for this feature.

I didn't see any data in new txt dump files with other values than chromosomal 0 doesn't that mean we don't have any data with a circular or non chromosomal sequence?

@dpopleton
Copy link
Contributor Author

there is no data in tests related to this feature. Shouldn't you add some?

Please check out the changes in the tests. There is full coverage for this feature.

I didn't see any data in new txt dump files with other values than chromosomal 0 doesn't that mean we don't have any data with a circular or non chromosomal sequence?

See src/tests/databases/core_1/attrib_type.txt
and src/tests/databases/core_1/seq_region_attrib.txt
These are tested in test new organism which has three sequences. One is marked as circular, one is marked as not circular, while the third has no circular attribute.
These are each tested in that first test. Here is an example.
sequence = session.query(AssemblySequence).where(
(AssemblySequence.is_circular == 1) & (AssemblySequence.name == 'TEST1_seq')
).first()
assert sequence is not None
There is also a single test for sequence.type within the same logic. It does not need any additional data as it extracts from coord_system which is already present.
assert sequence.type == "primary_assembly"

@marcoooo
Copy link
Contributor

marcoooo commented Dec 6, 2023

there is no data in tests related to this feature. Shouldn't you add some?

Please check out the changes in the tests. There is full coverage for this feature.

I didn't see any data in new txt dump files with other values than chromosomal 0 doesn't that mean we don't have any data with a circular or non chromosomal sequence?

See src/tests/databases/core_1/attrib_type.txt and src/tests/databases/core_1/seq_region_attrib.txt These are tested in test new organism which has three sequences. One is marked as circular, one is marked as not circular, while the third has no circular attribute. These are each tested in that first test. Here is an example. sequence = session.query(AssemblySequence).where( (AssemblySequence.is_circular == 1) & (AssemblySequence.name == 'TEST1_seq') ).first() assert sequence is not None There is also a single test for sequence.type within the same logic. It does not need any additional data as it extracts from coord_system which is already present. assert sequence.type == "primary_assembly"

Ok, this is misunderstanding here, I meant there is no unit test in place to check we load / update the right values from test set.

@dpopleton dpopleton requested a review from ens-st3 December 7, 2023 14:58
@dpopleton dpopleton merged commit 42993ec into main Dec 7, 2023
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.

4 participants