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

Improve predictor module #562

Merged

Conversation

elcorto
Copy link
Member

@elcorto elcorto commented Jul 29, 2024

Some cleanup / update in network/predictor.py that I'd like to move out of feature branches into develop, in preparation of a larger PR later which includes one feature only.

Must have been left out during merge of formatted develop branch
(6629b05).
Replace code duplication by slice object.
_forward_snap_descriptors() needs a Tensor but that isn't enforced.
The linting CI thinks this is necessary. Well ok.
@elcorto elcorto self-assigned this Jul 29, 2024
Copy link
Member

@RandomDefaultUser RandomDefaultUser left a comment

Choose a reason for hiding this comment

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

Hi @elcorto thanks for the improvements, I left a two comments for clarification, apart from that this looks good to me!

mala/network/predictor.py Show resolved Hide resolved
mala/network/predictor.py Outdated Show resolved Hide resolved
@RandomDefaultUser
Copy link
Member

I was wondering, should we address the second part of #508 in this PR as well? As you pointed out there, modifying the check should not be difficult.

@elcorto
Copy link
Member Author

elcorto commented Oct 18, 2024

I was wondering, should we address the second part of #508 in this PR as well? As you pointed out there, modifying the check should not be difficult.

Ideally yes, but I left it for discussion and did not change anything since I was unsure how to change the check.

@RandomDefaultUser
Copy link
Member

I was wondering, should we address the second part of #508 in this PR as well? As you pointed out there, modifying the check should not be difficult.

Ideally yes, but I left it for discussion and did not change anything since I was unsure how to change the check.

Got it, I actually looked into it and do no longer think this part of the code is redundant. The additional check makes sure that if the user specified an inference grid AFTER loading of the predictor class, this change is taken into account. Since the predictor object is loaded and the parameters object is not created beforehand, if you want to use a different inference grid this is the only way (I think?). The part in predict_from_qeout is still redundant, since the check should (and does) happen in predict_from_atoms, so this PR is still good to go from my side, if the CI runs for the docs.
I would close #508 though afterwards, I can add this exact comment to the issue to document it.

@elcorto
Copy link
Member Author

elcorto commented Oct 18, 2024

I was wondering, should we address the second part of #508 in this PR as well? As you pointed out there, modifying the check should not be difficult.

Ideally yes, but I left it for discussion and did not change anything since I was unsure how to change the check.

Got it, I actually looked into it and do no longer think this part of the code is redundant. The additional check makes sure that if the user specified an inference grid AFTER loading of the predictor class, this change is taken into account. Since the predictor object is loaded and the parameters object is not created beforehand, if you want to use a different inference grid this is the only way (I think?). The part in predict_from_qeout is still redundant, since the check should (and does) happen in predict_from_atoms, so this PR is still good to go from my side, if the CI runs for the docs. I would close #508 though afterwards, I can add this exact comment to the issue to document it.

OK thanks for the double check. Let's go ahead and close #508 then with this PR.

@RandomDefaultUser
Copy link
Member

Sounds good, will merge here.

@RandomDefaultUser RandomDefaultUser merged commit 49ed05f into mala-project:develop Oct 18, 2024
5 checks passed
@elcorto elcorto deleted the feature-improve-predictor branch October 18, 2024 14:40
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.

2 participants