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

Write dimension labels to DatasetBuilder on build #1081

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Conversation

rly
Copy link
Contributor

@rly rly commented Mar 25, 2024

Motivation

Fix #1077

How to test the behavior?

pytest

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Does the PR clearly describe the problem and the solution?
  • Have you reviewed our Contributing Guide?
  • Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

@rly rly requested a review from oruebel March 25, 2024 18:28
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.89%. Comparing base (77be0cc) to head (5446b60).
Report is 26 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1081      +/-   ##
==========================================
+ Coverage   88.83%   88.89%   +0.05%     
==========================================
  Files          45       45              
  Lines        9784     9833      +49     
  Branches     2779     2794      +15     
==========================================
+ Hits         8692     8741      +49     
  Misses        776      776              
  Partials      316      316              

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

@@ -15,6 +15,13 @@ class IncorrectQuantityBuildWarning(BuildWarning):
pass


class IncorrectDatasetShapeBuildWarning(BuildWarning):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these warnings should really be errors because they result in an invalid file. @oruebel what do you think? Should we upgrade all of these warnings to errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All except for DtypeConversionWarning and the generic BuildWarning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And maybe also the OrphanContainerWarning.

@rly rly requested review from mavaylon1 and removed request for oruebel July 23, 2024 18:12
@mavaylon1
Copy link
Contributor

@rly can you move BuildDatasetShapeMixin to a more general location? When importing this mixin class, it would be cleaner to have it a test_utils.py or test_mixin.py.

Otherwise, looks good.

@rly
Copy link
Contributor Author

rly commented Jul 24, 2024

@mavaylon1 The place to move it would be https://github.com/hdmf-dev/hdmf/blob/dev/tests/unit/helpers/utils.py. However, moving that means also moving the container classes and mappers defined in this file tests/unit/build_tests/mapper_tests/test_build.py. We might then want to resolve any redundancies between the Bar, Foo, and Baz test classes. I can do that, but I think this task fits well under the test harness refactoring that you wanted to do. By keeping it here for now, it's clear that this mixin is used only in this file. Should I move it, or let you take care of this as part of the larger refactoring?

@rly
Copy link
Contributor Author

rly commented Jul 25, 2024

TODO: I will look into the coverage failure

@rly rly enabled auto-merge (squash) July 25, 2024 23:43
@rly rly merged commit 4c32820 into dev Jul 26, 2024
26 checks passed
@rly rly deleted the dimension_labels_on_write branch July 26, 2024 21:42
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.

[Feature]: Store dimension labels in DatasetBuilder for backends to write
2 participants