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

Compare shapes in RunDescriber equality method #3105

Merged
merged 2 commits into from
Jun 9, 2021

Conversation

astafan8
Copy link
Contributor

@astafan8 astafan8 commented Jun 9, 2021

@jenshnielsen what do you think, should this be happening or not?

for latest version of the class, this seems very fair, but i'm not sure what should happen for the case where RunDescriber from V2 data (without shapes, because there were no notion of shapes back then) is compared with RunDescriber from V3 data (that has the shapes or has None as the shape) and the interdependencies part is equal. or am i overthinking this too much?

@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #3105 (bcf0bd8) into master (ccda37b) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #3105      +/-   ##
==========================================
- Coverage   65.66%   65.65%   -0.01%     
==========================================
  Files         216      216              
  Lines       28748    28750       +2     
==========================================
+ Hits        18876    18877       +1     
- Misses       9872     9873       +1     

@jenshnielsen
Copy link
Collaborator

One option could be to only to perform this comparison if both are not None? I am not sure how likely it is that someone has a v2 rundescriber that is upgraded and try to compare it with a directly constructed v3 describer

@jenshnielsen
Copy link
Collaborator

I think #3106 should fix the docs build here

@astafan8
Copy link
Contributor Author

astafan8 commented Jun 9, 2021

One option could be to only to perform this comparison if both are not None?

yeah, but that will also rouing the comparison of directly constructed

I am not sure how likely it is that someone has a v2 rundescriber that is upgraded and try to compare it with a directly constructed v3 describer

indeed. but if this is not likely, then the way the comparison is implemented in this PR is good, right?

@astafan8 astafan8 merged commit 8b92b4a into master Jun 9, 2021
@astafan8 astafan8 deleted the astafan8-compare-shapes branch June 9, 2021 15:58
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.

None yet

2 participants