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.

Sign up for GitHub

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

Jump to bottom

bugfix/data bands list ungrouped #5050

Merged
merged 1 commit into from
Sep 22, 2021
Merged

bugfix/data bands list ungrouped #5050

merged 1 commit into from
Sep 22, 2021

Conversation

dev-zero
Copy link
Contributor

@dev-zero dev-zero commented Aug 4, 2021 •
edited by sphuber
Loading

Fixes #5049

  • abstractqueries/get_bands_and_parents_structure: list ungrouped BandsData

@dev-zero dev-zero force-pushed the bugfix/data-bands-list-ungrouped branch from 035d32f to fa83575 Compare August 4, 2021 13:53
@codecov
Copy link

codecov bot commented Aug 4, 2021 •
edited
Loading

Codecov Report

Merging #5050 (a60c239) into develop (f952009) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5050      +/-   ##
===========================================
+ Coverage    80.89%   80.90%   +0.02%     
===========================================
  Files          536      536              
  Lines        37069    37073       +4     
===========================================
+ Hits         29982    29989       +7     
+ Misses        7087     7084       -3
Flag Coverage Δ
django 75.74% <100.00%> (+0.01%) ⬆️
sqlalchemy 74.84% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/orm/nodes/data/array/bands.py 77.46% <100.00%> (+0.48%) ⬆️
aiida/transports/plugins/local.py 81.66% <0.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f952009...a60c239. Read the comment docs.

@dev-zero dev-zero force-pushed the bugfix/data-bands-list-ungrouped branch from fa83575 to cca46fd Compare August 4, 2021 14:09
@dev-zero dev-zero requested a review from unkcpz August 4, 2021 14:24
@sphuber
Copy link
Contributor

sphuber commented Aug 4, 2021 •
edited
Loading

So that answers that long standing question of "who the hell uses these old abstract queries" 😄 To be honest though, I always felt that these shouldn't really be in aiida-core. It is one thing to have domain specific data types in there, but to have these very use case specific queries baked in feels off. They also go way back when there wasn't a single query builder frontend for both database backends. Since we are not removing the data types for v2.0, I guess we can keep this as well, so ok to fix this. But since it concerns a bug, it would be good to add a test for it. I think these abstract queries are notoriously under tested.

EDIT: I see now that this is actually used by the verdi data bands list command, so I guess that explains why you came across this bug.

@sphuber sphuber mentioned this pull request Aug 31, 2021
Merged
2 tasks
@dev-zero dev-zero force-pushed the bugfix/data-bands-list-ungrouped branch from cca46fd to c9ba160 Compare September 14, 2021 11:13
@dev-zero
Copy link
Contributor Author

rebased and test added for the first half of the get_bands_and_parents_structure (which this fix touches)

@dev-zero @sphuber
get_bands_and_parents_structure: also match ungrouped nodes
a60c239 
The query relied on `BandsData` nodes to be in a `Group` due to the way
the query was constructed because the join always happened through
`Group` regardless if the arguments specified group filters. This
resulted in ungrouped `BandsData` nodes to never be matched.

Co-Authored-By: Sebastiaan Huber <mail@sphuber.net>
@sphuber sphuber force-pushed the bugfix/data-bands-list-ungrouped branch from c9ba160 to a60c239 Compare September 22, 2021 10:07
@sphuber sphuber self-requested a review September 22, 2021 10:34
sphuber
sphuber approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @dev-zero . I have updated the tests to use pytest style instead. The AiidaTestCase method is being phased out.

@sphuber sphuber merged commit 35b5c34 into aiidateam:develop Sep 22, 2021
@dev-zero dev-zero deleted the bugfix/data-bands-list-ungrouped branch September 22, 2021 10:47
@chrisjsewell chrisjsewell mentioned this pull request Mar 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sphuber sphuber sphuber approved these changes

@unkcpz unkcpz Awaiting requested review from unkcpz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

verdi data bands list lists only grouped band structures
2 participants
@dev-zero @sphuber