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

TermSet minor test updates, _repr_html_, name field #967

Merged
merged 20 commits into from
Oct 27, 2023
Merged

TermSet minor test updates, _repr_html_, name field #967

merged 20 commits into from
Oct 27, 2023

Conversation

mavaylon1
Copy link
Contributor

@mavaylon1 mavaylon1 commented Oct 17, 2023

Motivation

What was the reasoning behind this change? Please explain the changes briefly.

  • Add repr_html to Termset
  • Add name to Termset
  • Update tests to include name

How to test the behavior?

Show how to reproduce the new behavior (can be a bug fix or a new feature)

Run notebook for html representation and then the unit tests.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running ruff from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@mavaylon1
Copy link
Contributor Author

Fix #893

@mavaylon1
Copy link
Contributor Author

It's been a while since #893 but we don't have versions anymore, the prefixes can be called by "termset.sources", I added the name, I added repr_html

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (75c686a) 88.59% compared to head (42488be) 88.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #967      +/-   ##
==========================================
+ Coverage   88.59%   88.65%   +0.06%     
==========================================
  Files          45       45              
  Lines        9424     9451      +27     
  Branches     2684     2688       +4     
==========================================
+ Hits         8349     8379      +30     
+ Misses        760      757       -3     
  Partials      315      315              
Files Coverage Δ
src/hdmf/term_set.py 98.23% <100.00%> (+2.43%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mavaylon1 mavaylon1 changed the title Quality of life changes for Termset TermSet minor test updates, _repr_html_, name field Oct 18, 2023
@mavaylon1 mavaylon1 self-assigned this Oct 18, 2023
@mavaylon1
Copy link
Contributor Author

I did not move termset and herd to a folder as the issue ticket suggested. That would require a lot of import changes for something that doesn't provide much value to the user, nor me as the main person who works on this.

@mavaylon1 mavaylon1 marked this pull request as ready for review October 18, 2023 22:05
@mavaylon1 mavaylon1 requested a review from oruebel October 18, 2023 22:05
src/hdmf/term_set.py Outdated Show resolved Hide resolved
@rly
Copy link
Contributor

rly commented Oct 19, 2023

The YAML schema in TermSet.term_schema_path already has a name. I think it is required but I am not sure. Could we extract the name field from the schema? I think that is what I meant in #893. Add convenience properties name, version, and prefixes that allow the user to get those values from the YAML schema.

src/hdmf/term_set.py Outdated Show resolved Hide resolved
@mavaylon1
Copy link
Contributor Author

The YAML schema in TermSet.term_schema_path already has a name. I think it is required but I am not sure. Could we extract the name field from the schema? I think that is what I meant in #893. Add convenience properties name, version, and prefixes that allow the user to get those values from the YAML schema.

That makes sense to extract.

@mavaylon1 mavaylon1 requested a review from rly October 19, 2023 15:35
@rly
Copy link
Contributor

rly commented Oct 27, 2023

@mavaylon1 thanks for the PR. I think printing "[item 1] [item 2] ... [item 3]" and "[item 1] [item 2] ... [item 3] [item 4]" when there are only 3 or 4 items in the list is misleading, so I changed the format a bit so that the ellipses only show up if there are more items present than are shown. Super minor, but I think it is better this way.

Also, the panda that you added to your example is a genus, not a species, so I updated that accordingly.

@mavaylon1 mavaylon1 merged commit 56956ae into dev Oct 27, 2023
26 checks passed
@mavaylon1 mavaylon1 deleted the repr branch October 27, 2023 13:22
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