Enable installing the python helpers with pip #96

Luthaf · 2020-11-17T15:39:09Z

One unresolved question is what should be the name of the package. I went with chemiscope, but this might create a bit of confusion with the npm/javascript chemiscope package. An alternative would be chemiscope-input, but then if/when we create a jupyter plugin for chemiscope it would have to live in a different package.

Luthaf · 2020-11-17T16:22:24Z

@rosecers do you think you could give this a look?

ceriottm · 2020-11-17T16:25:41Z

A few thoughts.

This is important, and an under-developed part of chemiscope. We should make it as easy as possible to generate chemiscopes as part of a ML analysis workflow. As far as I understand, currently the workflow goes through creating an ASE frames that store the various properties - it is unclear to me if this allows exploiting fully the features of chemiscope, see e.g. the definition of units or property descriptions
Something like chemiscope-python, for the package name ?

Luthaf · 2020-11-17T16:43:31Z

As far as I understand, currently the workflow goes through creating an ASE frames that store the various properties

That's only true for the command line version. The function supports an extra argument (should probably be renamed to properties) to specify arbitrary properties, including the newly added description and units. I've also started to decouple these scripts from ASE to allow for other structure/properties backends.

We should make it as easy as possible to generate chemiscopes as part of a ML analysis workflow

Completely agree, but I'm not sure we want to tackle this in this repository, and definitively not in this PR =)

This was the goal of our initial idea with @rosecers for the chemistack proposal, and seems to be related to what I understand M-stack would be. The issue here is that there are a lot of different ways one could want to create the map part of chemiscope. While at COSMO we use SOAP/librascal + and a few different mapping algorithms, there is no reason to marry chemiscope with these specific descriptors and mapping algorithms if we want it to be used by the widest variety researchers.

We could create a prototype chemistack easily using notebooks, @rosecers already has one prototype using matplotlib to display the map. This also links into the FlowingSOAP idea at materials cloud. Overall I think we should have a meeting/brainstorming session to decide what we want to do here.

There is also the ASAP tool to so something like this, so we could also help and work with them instead of re-inventing our own wheel.

Something like chemiscope-python, for the package name ?

I think I prefer having the same name in pip and in code (so pip install XXX and then from XXX import), and from chemiscope_python import write_input feels a bit strange. Overall I'm not opposed to chemiscope as a name for the Python package as well, do you think there could be confusion with the npm package with the same name?

ceriottm · 2020-11-17T17:44:55Z

We should make it as easy as possible to generate chemiscopes as part of a ML analysis workflow

Completely agree, but I'm not sure we want to tackle this in this repository, and definitively not in this PR =)

This PR I agree, this repo we should. I don't mean that the analysis should be part of this repo, but that we should make converting the output of the analysis into a chemiscope as seamless as possible. So M-stack is a separate discussion, here I just wanted to stress that the process of getting an xyz and an analysis output into a chemiscope could be easier.

Something like chemiscope-python, for the package name ?

I think I prefer having the same name in pip and in code (so pip install XXX and then from XXX import), and from chemiscope_python import write_input feels a bit strange. Overall I'm not opposed to chemiscope as a name for the Python package as well, do you think there could be confusion with the npm package with the same name?

My thinking is probably less informed than yours. To me it's not the end of the world to have a pip chemiscope that differs from npm chemiscope but perhaps someone better informed has discussed the case and come up with good practices?

Read the json file to access it in setup.py, and use setuptools to set chemiscope.__version__ accordingly

Luthaf · 2020-11-18T10:32:40Z

I don't mean that the analysis should be part of this repo, but that we should make converting the output of the analysis into a chemiscope as seamless as possible. So M-stack is a separate discussion, here I just wanted to stress that the process of getting an xyz and an analysis output into a chemiscope could be easier.

Oh, right 😄. Completely agree with this, do you have specific pain points that could be improved? Or an ideal workflow that we could try to enable?

By analysis output, do you mean a in-memory numpy array, or some data save on the disk?

ceriottm · 2020-11-18T12:29:16Z

Oh, right 😄. Completely agree with this, do you have specific pain points that could be improved? Or an ideal workflow that we could try to enable? By analysis output, do you mean a in-memory numpy array, or some data save on the disk?

I think ideally we should keep the dual workflow with a function that is mirrored by a command-line utility. People from "my generation" still have an instinct to go full bash onto postprocessing. So I think we want to be able to easily combine structures (here having something that can be read by ASE or an Atoms list seem to cover quite some grounds) and arrays of values (that maps easily into column files) or dicts. One thing that often bugs me is that I want to drop info from the ASE file so there could also be a switch that allows you to drop those fields. I don't know if you get the idea or want to discuss a more formal API/set of use cases.

rosecers

This in general looks good -- there were a few places that I have questions and suggestions, but there's nothing prohibitive.

python/chemiscope/adapters.py

python/chemiscope/input.py

rosecers · 2020-11-18T12:55:01Z

python/chemiscope/main.py

+    parser.add_argument(
+        "--description", default="", type=str, help="description of the dataset"
+    )


Could we also make it possible to load the description from a text file? Seems likely that this will be a very long argument.

The command line script is more for quick & dirty JSON generation. I would rather make people use the python function directly instead if the command line script is not enough for them.

rosecers · 2020-11-18T12:55:31Z

python/chemiscope/main.py

+    if args.only_structures and args.cutoff is not None:
+        raise Exception("--only-structure can not be given with --cutoff")


Should this be a warning, rather than an exception?

Why? It is bogus, there is no obvious way to recover (should cutoff or only-structure take precedence?), and the user can correct this easily by changing the command line invocation.

Luthaf · 2020-11-18T16:09:01Z

One thing that often bugs me is that I want to drop info from the ASE file so there could also be a switch that allows you to drop those fields.

I think I get the idea, that should already be possible within python with del frame.info["whatever"] or del frame.arrays["whatever"]. We could add something like this to the script. I'll open a separate issue to discuss this, so that it does not ends up buried in the PR comments.

Luthaf · 2020-11-19T15:24:18Z

So I went a bit ahead and added unit tests for most of the python code. This should help to improve it later, but makes this PR a bit large. I'll try not to add anything else to it =)

python/chemiscope/adapters.py

python/chemiscope/input.py

python/chemiscope/adapters.py

Luthaf · 2020-11-20T16:51:02Z

Everything should be addressed!

rosecers

Looks good to me. On the record, I'd like us to revisit in the future the decision to omit unexpected/incomplete entries.

Luthaf · 2020-11-20T17:02:13Z

On the record, I'd like us to revisit in the future the decision to omit unexpected/incomplete entries.

As soon as we have support for them (#38), I would agree! See also #97 (comment) on this subject.

Luthaf added 4 commits November 17, 2020 16:36

Create a python package containing write_chemiscope_input

87c641c

Partially decouple chemiscope.input from ASE

fc773af

Do not rename property XXX to XXX[1] if XXX shape is (n, 1)

8ce3603

Update documentation

8f9acfe

Luthaf requested a review from ceriottm November 17, 2020 15:39

Centralize the version number to package.json

ca72275

Read the json file to access it in setup.py, and use setuptools to set chemiscope.__version__ accordingly

rosecers approved these changes Nov 18, 2020

View reviewed changes

Luthaf mentioned this pull request Nov 18, 2020

Make creating chemiscope input easier #97

Closed

Luthaf added 4 commits November 19, 2020 11:35

Fix installation of python package from a sdist

265190e

Separate the input creating and file writting

21ca33f

Rename extra to properties

f239077

Add unit tests for create_input and write_input

9d1e3a6

This comment has been minimized.

Sign in to view

Deal with frames having different properties

584d905

Luthaf force-pushed the pip-install branch from 73d60de to 808b841 Compare November 19, 2020 15:32

Run python unit tests on CI

ce0839e

Luthaf force-pushed the pip-install branch from 808b841 to ce0839e Compare November 19, 2020 15:33

Luthaf requested a review from rosecers November 19, 2020 16:34

rosecers reviewed Nov 20, 2020

View reviewed changes

rosecers self-requested a review November 20, 2020 14:34

Address review comments

ced8790

rosecers approved these changes Nov 20, 2020

View reviewed changes

Luthaf merged commit 3b61a37 into lab-cosmo:master Nov 20, 2020

Luthaf deleted the pip-install branch November 20, 2020 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable installing the python helpers with pip #96

Enable installing the python helpers with pip #96

Luthaf commented Nov 17, 2020

Luthaf commented Nov 17, 2020

ceriottm commented Nov 17, 2020

Luthaf commented Nov 17, 2020

ceriottm commented Nov 17, 2020

Luthaf commented Nov 18, 2020

ceriottm commented Nov 18, 2020 via email

rosecers left a comment

rosecers Nov 18, 2020

Luthaf Nov 18, 2020

rosecers Nov 18, 2020

Luthaf Nov 18, 2020

Luthaf commented Nov 18, 2020

Luthaf commented Nov 19, 2020

This comment has been minimized.

Luthaf commented Nov 20, 2020

rosecers left a comment

Luthaf commented Nov 20, 2020

		if args.only_structures and args.cutoff is not None:
		raise Exception("--only-structure can not be given with --cutoff")

Enable installing the python helpers with pip #96

Enable installing the python helpers with pip #96

Conversation

Luthaf commented Nov 17, 2020

Luthaf commented Nov 17, 2020

ceriottm commented Nov 17, 2020

Luthaf commented Nov 17, 2020

ceriottm commented Nov 17, 2020

Luthaf commented Nov 18, 2020

ceriottm commented Nov 18, 2020 via email

rosecers left a comment

Choose a reason for hiding this comment

rosecers Nov 18, 2020

Choose a reason for hiding this comment

Luthaf Nov 18, 2020

Choose a reason for hiding this comment

rosecers Nov 18, 2020

Choose a reason for hiding this comment

Luthaf Nov 18, 2020

Choose a reason for hiding this comment

Luthaf commented Nov 18, 2020

Luthaf commented Nov 19, 2020

This comment has been minimized.

Luthaf commented Nov 20, 2020

rosecers left a comment

Choose a reason for hiding this comment

Luthaf commented Nov 20, 2020