search

libAtoms / workflow

python workflow toolkit
GNU General Public License v2.0
35 stars 18 forks source link

deprecate things that are not being done properly #44

Closed bernstei closed 2 years ago

bernstei commented 2 years ago

Deprecate bits of code that don't do things the workflow way, or just don't use it. They can be updated to do it the right way (so they can be wrapped by iterable_loop (soon to be autoparallelize), or moved to some user-specific place.

reactions_processing/ plotting/ some things in generate, e.g. neb, ts, vib, radicals

some things in cli.py

hheenen commented 2 years ago

Considering the plotting/, I understand this is not linked to the autoparallelize, but I wonder if you don't want to provide some standard plots (one may add more) which could help users to monitor progress of potential training?

If this is not wanted in the wfl/ code maybe it makes sense to move it out of it, but keep it separately in the repo?

bernstei commented 2 years ago

I don't even know what's in plotting/, but maybe we need a different repo for analysis?

hheenen commented 2 years ago

I mean that's up to you and maybe a separate repo is easier to maintain. I personally think that providing some analysis tools for workflow in the same repo would be more convenient, though. So far some of us have been using functions like plot_ef_correlation.py producing detailed energy and force correlation plots. Of course we could generalize and refactor the contained functions and add some more of our own which could fit in there -- we also happily do this if you chose to make a separate analysis repo.

bernstei commented 2 years ago

@gabor1 where should we put these things?

gelzinyte commented 2 years ago

Just to collect some other things to revise:

gelzinyte commented 2 years ago

@bernstei what's wrong with vib.py?

bernstei commented 2 years ago

It's sort of OK, since it does use ConfigSet, but wouldn't it be more modular to have functions that just generate the necessary configurations, then use the existing autoparallelized generic eval loop, and then routines that analyze them? Given that the calculations will almost certainly dominate the cost that seems like a more useful packaging of the work.

Zausinator commented 2 years ago

Very small nitpick, but might I suggest that all style elements be removed from the plotting section? Seems like that is something that the users can easily set via their plt.rcParams. So instead of defining a sequence of colors, we just define:

colors = plt.rcParams['axes.prop_cycle'].by_key()['color']

and iterate through those parameters. The figsizes, labelsizes, etc. would then all be taken directly from the user's individual setups. The added benefit would be that the code becomes a lot shorter.

An alternative approach would be to provide a default rcParams file somewhere, which would then be the fallback.

gabor1 commented 2 years ago

providing a default rcParams would be good. not everyone controls their styles by rcParams files

bernstei commented 2 years ago

Comments on what's there now:

I think that vib has a good reason to stay, but all the others should probably be moved outside of wfl, whether that's some user or system specific directory distributed with the workflow, or entirely separate. I've tentatively moved them to workflow/deprecated.

As for vib, we should think about the best interface, and perhaps also making it uniform with the equivalent functionality for periodic systems (using phonopy). I have routines for that, but would need to think about how to unify the two interfaces.

bernstei commented 2 years ago

Current plan:

@gabor1 sounds OK to you? Do we have a better name for the user directory for Tamas's code?

stenczelt commented 2 years ago

I am OK with deprecating them, if we even want to re-use them then git remembers everything. Feel free to go ahead with purging this part of the codebase.

On 5 May 2022, at 19:21, bernstei @.***> wrote:

Current plan:

reactions_processing, neb, ts, radicals to user/reactions (name to be finalized) plotting to deprecate/plotting, to be moved into a separate directory or repo for analysis vib unchanged for now, pending thought about merging with phononpy based code for periodic systems. @gabor1 https://github.com/gabor1 sounds OK to you? Do we have a better name for the user directory for Tamas's code?

— Reply to this email directly, view it on GitHub https://github.com/libAtoms/workflow/issues/44#issuecomment-1118908578, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJKZU7CGWPSOUV5EIJU3JMDVIQGUBANCNFSM5RLRZ5EA. You are receiving this because you were mentioned.