-
Notifications
You must be signed in to change notification settings - Fork 585
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
Fix error in 'qml.data.load()` when using 'full' parameter value #4663
Fix error in 'qml.data.load()` when using 'full' parameter value #4663
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4663 +/- ##
=======================================
Coverage 99.64% 99.64%
=======================================
Files 376 376
Lines 33828 33833 +5
=======================================
+ Hits 33707 33712 +5
Misses 121 121
☔ View full report in Codecov by Sentry. |
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.
looks good! just clarifying something you said:
...
basis=['STO-3G', 'CC-PVDZ'], bondlength=['0.5', '0.7'])
will return 4 datasets. But if one those is removed, 3 will be returned without throwing an error.
if one what is removed? and which 3 will be returned? I'm not entirely sure what the expected behaviour is there.
So before this PR, if There's not really an 'expected' behaviour here (afaik) so we're not breaking any promises. |
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.
ahh gotcha - you mean removed from the actual database, makes sense! looks great
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.
Looks good to me, not getting the error anymore 💯
Context:
qml.data.load()
did not properly handle specified parameters that come after'full'
. For example, the followingcall should return all qspin datasets matching
sysname='Heisenberg', periodicity='open', layout='2x2'
, for any valueof
lattice
:Instead, we get an exception, even though matching datasets do exist:
This happens because the
find()
function fails as soon as it can't find a matching dataset for any of the parameters. When it checks the'chain'
lattice, it can't find'2x2'
and throws an exception, even though'2x2'
is available for'rectangular
'.Description of the Change:
Foldermap.find()
will only throw an exception if it cannot find any matching datasets for a given parameter. This does mean that the user can provide invalid parameters, as long as one of them matches. For example:Will not result in an error.
Benefits:
qml.data.load()
can handle full parameters that come before specific parameters.Possible Drawbacks:
qml.data.load('qchem', molname='H2', basis=['STO-3G', 'CC-PVDZ'], bondlength=['0.5', '0.7'])
will return 4 datasets. But if one those is removed, 3 will be returned without throwing an error.Related GitHub Issues:
Fixes #4270