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

Move calcfunctions to aiida_quantumespresso.calculations.functions #520

Merged
merged 1 commit into from
May 7, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented May 7, 2020

Fixes #518

Since they are calculation functions they do not belong in the workflow
module. To not break backwards-compatibility the original modules import
the functions from their new location and emit a Warning. Note that we
explicitly do not use a DeprecationWarning since those are
automatically filtered in Python 3.

@sphuber sphuber requested a review from greschd May 7, 2020 14:18
@greschd
Copy link
Member

greschd commented May 7, 2020

I think that should be a FutureWarning, based on how it's described here: https://docs.python.org/3/library/exceptions.html#FutureWarning.
Of course DeprecationWarning will still be shown e.g. by pytest, but I agree we are closer to an "end-user" scenario here, since we don't expect all code using it to run through a test suite.

I checked that FutureWarning is not silenced by default in py3.

@greschd
Copy link
Member

greschd commented May 7, 2020

The test failure is on me, the test should probably filter out calcfunctions 😅

@sphuber
Copy link
Contributor Author

sphuber commented May 7, 2020

I think the way the words "user" and "developer" are being used by the Python docs is a bit different from our usage though. I want this warning to be seen by people using the aiida-quantumespresso plugin, but really they will be "developing" their own plugin or at least AiiDA-based code. I am guessing their reasoning is that developers will be reading the source code of libraries they are using (optimistic but okay) and so they will see it there and therefore it can be suppressed by default. My interpretation of the FutureWarning is that if you need to warn about deprecations that will even reach end-users. But for this change here, I need to reach everyone. But anyway, in the end it does not really matter. I will use FutureWarning given that it is more specific than just a general Warning. Will also fix the test :)

@sphuber sphuber force-pushed the fix/518/move-calcfunctions branch from 64eba0d to 9de0579 Compare May 7, 2020 15:23
@sphuber
Copy link
Contributor Author

sphuber commented May 7, 2020

@greschd fixed the tests and change the Warning to FutureWarning

Since they are calculation functions they do not belong in the workflow
module. To not break backwards-compatibility the original modules import
the functions from their new location and emit a `Warning`. Note that we
explicitly do not use a `DeprecationWarning` since those are
automatically filtered in Python 3.
@sphuber sphuber force-pushed the fix/518/move-calcfunctions branch from 9de0579 to fd725da Compare May 7, 2020 21:32
@sphuber sphuber merged commit 72b3e9a into develop May 7, 2020
@sphuber sphuber deleted the fix/518/move-calcfunctions branch May 7, 2020 21:39
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.

Move calculation functions to aiida_quantumespresso.calculations
2 participants