-
Notifications
You must be signed in to change notification settings - Fork 187
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
isolated bandsdata list and test #3817
Conversation
Emm.. I think I don't have the permissions to assign reviewers, right? Could you review it?@ltalirz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot @unkcpz for the fix!
some questions/comments from my side.
Re pre-commit failure: it's probably due to a different yapf version - aiida-core currently uses 0.28
@@ -136,7 +136,7 @@ def _extract_formula(args, akinds, asites): | |||
|
|||
# We want only the StructureData that have attributes | |||
if akinds is None or asites is None: | |||
return None | |||
return '<<UNKNOWN>>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand: why do we need to introduce this custom label?
EDIT: Oh, I see, it is already in the code... Aren't you changing the meaning of <<UNKNOWN>>
here, though?
Anyhow, since you now already looked into this function, if you could add a few lines of documentation to its docstring that would be great.
Have you seen whether this args
object is documented anywhere else?
We're really digging in some aiida code from the "old days" here... great to clean this up a little bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you are right. I did this because I want to make as few changes as possible.
Now, I use NOT FOUND
for the isolated bands, and reserve <<UNKNOW>>
for what was originally designed for (the situation when formula variable is unproperly assigned).
both backend supported
Thanks a lot @unkcpz for working through this convoluted code! Before I go through this line-by-line: While we're at it, there is also a django-specific implementation of aiida-core/aiida/backends/djsite/queries.py Lines 148 to 193 in 09cb6cb
|
@ltalirz @giovannipizzi If it is correctly, I can make a PR to try to fixes the band query function here and check if the other place have the same problem. |
I don't think this function is a leftover - it was moved there on purpose (I have some memory of this) because it was faster. Now, I don't know if with the current changes it's faster. Note that this touched the transitive table, i.e. now the ancestors and descendants in the QueryBuilder. I agree that that functionality is quite specific. However, before removing, I suggest testing the Django-specific implementation vs. the generic implementation, and reporting the results here (but this should be done in a big DB). |
The original purpose of this PR seems already reached. Could you merge this so I can get back to #3798 based on this PR? @ltalirz I have no experience at working several interrelated issues at the same time, could you tell me how to deal with this situation? :-p |
Too many Daniels in the world 😆 Except for one PR, I have no connection to this project. @unkcpz I think you wanted to mention @danieleongari, though I seem to be unable to mention him for some reason, though Github does not suggest him while typing for some reason... |
forgive my careless @danielhollas . yes, Github does not suggest him while typing. |
Codecov Report
@@ Coverage Diff @@
## develop #3817 +/- ##
===========================================
- Coverage 78.13% 77.21% -0.93%
===========================================
Files 460 458 -2
Lines 33978 33767 -211
===========================================
- Hits 26548 26072 -476
- Misses 7430 7695 +265
Continue to review full report at Codecov.
|
@CasperWA Just to understand - should codecov now be working on PRs from forks or not? |
I believe it should work. You can always check whether a codecov report was successfully uploaded by checking the CI run. See, e.g., here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot @unkcpz , and sorry for the large delay in review.
The changes look good to me, I have just some minor requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @unkcpz !
Fixes #3811
BandsData
objectverdi data bands list
returnsNOT FOUND
as formula.<<UNKNOW>>
reserved for the situation when ancestor found but its formula cannot be read properly.abstractqueries::get_bands_and_parents_structure
query for band after the group query to prevent item listed in empty group._extrat_formula
removed fromdjsite::queries
use the general function.