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

TestWrite.test_missing_entries() #533

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

joeflack4
Copy link
Collaborator

@joeflack4 joeflack4 commented May 20, 2024

addresses #534

Changes

  • Add: TestWrite.test_missing_entries()

@joeflack4 joeflack4 requested a review from matentzn May 20, 2024 01:19
@joeflack4 joeflack4 self-assigned this May 20, 2024
@joeflack4 joeflack4 added enhancement New feature or request bug Something isn't working and removed bug Something isn't working labels May 20, 2024
@joeflack4 joeflack4 force-pushed the missing-entries branch 2 times, most recently from ef955af to 97d9ab1 Compare May 20, 2024 01:41
# TODO: Determine assertions that should pass
# TODO: Fix failing assertions that should pass
# TODO: before merging, remove pass/fail comments at end of assertion lines
def test_missing_entries(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Get assertions to pass

  • 1. Determine assertions that should pass @matentzn
  • 2. Fix failing assertions that should pass
  • 3. before merging, remove pass/fail comments at end of assertion lines

# TODO: Determine assertions that should pass
# TODO: Fix failing assertions that should pass
# TODO: before merging, remove pass/fail comments at end of assertion lines
def test_missing_entries(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which assertions are appropriate?

Can you look at the assertions I've made and determine which ones you think should be expected to pass, and which should not?

I've left temporary comments next to each assertion showing their current pass/fail status.

"mapping_provider": "https://www.orpha.net/",
}

# When passing Converter
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI: 2 different setups

I have a set of assertions for 2 cases of MappingSetDataFrame instantiation:

  • with inclusion of a Converter
  • with no inclusion of a Converter

@joeflack4 joeflack4 added the bug Something isn't working label May 20, 2024
@joeflack4 joeflack4 marked this pull request as draft May 20, 2024 01:47
@joeflack4 joeflack4 force-pushed the missing-entries branch 3 times, most recently from 001557e to 43fa8f5 Compare May 20, 2024 01:53
Copy link
Collaborator

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

This is a very good test! Thank you!

tests/test_writers.py Outdated Show resolved Hide resolved
with open(tmp_file, "w") as f:
write_table(msdf, f)
msdf2 = parse_sssom_table(tmp_file)
self.assertIn("curie_map", msdf2.metadata) # fail
Copy link
Collaborator

Choose a reason for hiding this comment

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

This behaviour is expected for now, because curie_map sort if lives outside the metadata. however, it will soon be solved: mapping-commons/sssom#349

Copy link
Collaborator Author

@joeflack4 joeflack4 May 22, 2024

Choose a reason for hiding this comment

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

Wow, I'm surprised it wasn't part of the data model.

From an implementation standpoint, I was thinking that maybe msdf.prefix_map and msdf.metadata['curie_map'] could point to the same Python dictionary object. Right now they are different objects and, as these assertions show, can get out of sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestWrite.test_missing_entries()
2 participants