Add restricted insertions attribute for `moves` #47

rmatsum836 · 2020-04-01T20:34:45Z

Based on numerous discussions with @rsdefever, restricted_insertions is a useful argument to run GCMC or GEMC simulations of slit pores.

This PR adds restricted_insert as an attribute of the moves object. This is a work in progress.

Add checks to ensure restricted_insertions are valid
Add unit tests
Decide on appropriate data structures for this attribute

rsdefever · 2020-04-01T21:50:40Z

@rmatsum836 this looks like a great start! Let me know if I can help! I'm excited to get this feature supported.

rmatsum836 · 2020-04-08T16:17:31Z

Based on the conversation we had regarding #49, I made the arguments for restricted insertions lists. The black formatting test is failing (not surprised), but all of the tests are passing locally for me. I would also like to add a few more tests to ensure this addition is correctly integrated into the current code.

rmatsum836 · 2020-04-09T15:38:09Z

I need to rework this in order to specify restrictions by components in the box. Will try to have an update later today.

rmatsum836 · 2020-04-13T14:15:57Z

This is still a little rough around the edges but I think it's ready for a first review. Some things I'm unsure about:

Is the check to validate the restricted insertions vs box sufficient in inp_functions.py?
Are there any additional checks that need to be done?

rsdefever

Overall this looks good, and a very nice addition. A few overall comments:

Major:
(1) Can you add an example with restricted insertion to the examples directory and then add a test for the example.
(2) Create a moves object, and add a restricted_type, but don't add the restricted_value. Then try mc.print_inputfile(). You'll see it errors out when trying to write the input file. I think we should try to catch this error before then.
(3) I know you were following the "template" with the way I've handled other things associated with the moves object, but in this case would it be better to have an add_restricted_insertion or restrict_insertion function to add these restrictions? Somehow it feels more natural in my head. Interested in your opinion.

Minor:
(1) In the future, try to apply black formatting as you go (and ideally before commits), so we don't end up with a bunch of commit messages that are Apply black formatting.

rsdefever · 2020-04-13T20:25:33Z

mosdef_cassandra/core/moves.py

+                        len(restricted_type)))
+                if len(restricted_type[0]) != self._n_species:
+                    raise ValueError("{} species specified"
+                    ' but restricted_typed for only {}'


The word "only" only makes sense if len > n_species. I'd just remove it.

rsdefever · 2020-04-13T20:27:04Z

mosdef_cassandra/core/moves.py

+        if restricted_value:
+            if len(self.restricted_type) != len(restricted_value):
+                raise ValueError("'restricted_type' is a list of length"
+                        " {} but 'restricted_value' is a list of length"


Logically the comparison makes sense, but I think the error message could be a bit clearer. The underlying problem is that the user is specifying restricted values for the wrong number of boxes.

rsdefever · 2020-04-13T20:28:41Z

mosdef_cassandra/core/moves.py

+                        )
+            if not isinstance(restricted_value[0], (list, tuple)):
+                raise TypeError("'restricted_value' specified for"
+                " a box must be an iterable.")


The "must be an iterable" terminology, while technically correct, is, perhaps a bit confusing and not to-the-point. Something about a list or tuple with one element per species would be more clear IMO.

mosdef_cassandra/core/moves.py

rsdefever · 2020-04-13T20:36:01Z

mosdef_cassandra/tests/test_writers.py

+            run_length=500,
+            temperature=300.0,
+            pressure=1,
+            chemical_potentials=["none", 10.0],


We don't need to specify a chemical potential for GEMC. In fact we should add an error if someone tries for future.

rsdefever · 2020-04-13T20:36:20Z

mosdef_cassandra/tests/test_writers.py

+                run_length=500,
+                temperature=300.0,
+                pressure=1,
+                chemical_potentials=["none", 10.0],


Repeat about chemical potential

rsdefever · 2020-04-13T20:38:49Z

mosdef_cassandra/tests/test_writers.py

+        )
+
+        if typ == 'interface':
+            assert "\nrestricted_insertion {} {} {}\n".format(typ, value[0], value[1]) in inp_data


Have you done a check to confirm that this (and the other sections where you are testing the input writers) actually works for Cassandra? I.e., the formatting is actually correct. It looks correct but would rather be safe than sorry.

rsdefever · 2020-04-13T20:40:14Z

mosdef_cassandra/writers/inp_functions.py

@@ -241,7 +241,7 @@ def generate_input(system, moves, run_type, run_length, temperature, **kwargs):
            box_dims = np.hstack((box.lengths, box.angles))
        box_matrix = convert_box.convert_to_boxmatrix(box_dims)
        boxes.append(box_matrix)
-    inp_data += get_box_info(boxes)
+    inp_data += get_box_info(boxes, moves)


Do you have an opinion about passing the full moves object vs. just the restricted insertion details? I'm just thinking from a readability of the code standpoint it might be clearer to see exactly what part of the moves is need in writing the box info?

rsdefever · 2020-04-13T20:42:51Z

mosdef_cassandra/writers/inp_functions.py

@@ -1054,6 +1116,7 @@ def get_move_probability_info(**kwargs):
        "volume",
        "insert",
        "swap",
+        "restricted_insertion"


Can you add docstrings?

rmatsum836 · 2020-04-13T22:06:00Z

Thanks @rsdefever for the comments, I'll get to work on addressing them.

I know you were following the "template" with the way I've handled other things associated with the moves object, but in this case would it be better to have an add_restricted_insertion or restrict_insertion function to add these restrictions? Somehow it feels more natural in my head. Interested in your opinion.

In the comment above, are you referring to the way that restricted insertions get set in the Moves object? So would add_restricted_insertions be a method of Moves that adds restricted insertions into the object?

rsdefever · 2020-04-13T23:00:40Z

So would add_restricted_insertions be a method of Moves that adds restricted insertions into the object?

Yeah that was the thought anyway...it might be a bad idea. The interface could perhaps look something like:

moves.add_restricted_insert(species=species, box=box, restricted_type='sphere', radius=0.3)

But...this also starts to encroach on how to best handle the species and box spec (e.g., #49).

I'm also OK with leaving it as is for now and adding it to the list of things to think about. In my head, editing a move probability with something like moves.prob_swap = 0.2 makes good sense, but needing to specify the restricted_types and then separately restricted_values feels a bit more cumbersome. I'm not entirely sure what the difference is in my head? Maybe it's that the other attributes of the moves object come with default values. As much as anything I wanted to get some other feedback on these thoughts.

rmatsum836 · 2020-04-14T13:40:19Z

I kinda like this idea. I agree it's cumbersome to add both restricted_type and restricted_value and have to check that both attributes are compatible with each other. Similar to what you had above, I was thinking something like:

moves.add_restricted_insertion(species=species, restricted_types='sphere', values=0.3)

def add_restricted_insertion(species, restricted_types, values):
    self._restricted_types = restricted_types
    self._restricted_values = values

This makes sense to me because the restricted insertions seem like a very optional argument that won't get used most of the time. Also this allows a user to pass in the type and value of restricted insertions at the same time, but the values get set as separate attributes under the hood. The attributes will also be "private" so that a user doesn't try to directly specify self._restricted_types or self._restricted_values. Does that work?

rsdefever · 2020-04-14T21:02:34Z

That all sounds good. Questions:

(1) How do we specify the species and verify that it is valid (i.e., part of the species list)
(2) Same as (1), but for the box.

If we can cleanly address those two issues, I prefer have an add_restricted_insertion method. If those present currently insurmountable challenges, I am fine with pushing ahead by forcing the user to edit both attributes (i.e., the current implementation).

rmatsum836 · 2020-04-14T21:19:20Z

That I'm not sure about. Currently I'm trying to follow the current implementation in that if species is defined as [water, methane] and then the first index of restricted_type would correspond to water and the second would correspond to methane. We could specify and verify the species from species_topologies that exists as an attribute in the moves object.

For boxes however, I don't think moves carries box information so this would be a little bit tougher. moves does contain information for number of boxes though, so currently I'm just checking to make sure that the restricted insertions information matches up with the number of boxes.

rmatsum836 · 2020-04-15T17:23:30Z

I went ahead and added the method add_restricted_insertions to moves. I personally like this much better than the previous implementation, mainly because it's much easier to check that the restricted_type and restricted_value passed are compatible with each other.

I think I addressed your comments, including the addition of a gcmc_restricted example. If you like the add_resricted_insertions method as well, I will go ahead and delete the old implementation in moves.py that is currently commented out.

rsdefever · 2020-04-16T02:30:36Z

I like the new interface (i.e., method-based) much more. I think it is safe to delete the commented code. It looks like you've covered most of my requested changes. Once you're comfortable I'll give it one more look-over and then LGTM. 👍

I think long run I may need to consider overhauling System and Moves somewhat. But that is a separate issue and this looks great!

codecov-io · 2020-04-16T14:46:21Z

Codecov Report

Merging #47 into master will decrease coverage by 0.70%.
The diff coverage is 84.42%.

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage   95.48%   94.78%   -0.71%     
==========================================
  Files          20       21       +1     
  Lines        1463     1572     +109     
==========================================
+ Hits         1397     1490      +93     
- Misses         66       82      +16

rmatsum836 · 2020-04-16T15:02:37Z

moves.py has been cleaned up and I added more tests in test_moves.py to make sure the ValueErrors are raised as intended. I think this is ready for a second review.

One possible source of error I noticed is that we can create a moves object with a specific ensemble, add restricted insertions, and then change the ensemble like so:

moves = mc.Moves("gcmc", [methane_oplsaa])
moves.add_restricted_insertions([methane_oplsaa], [[typ]], value)
moves.ensemble = "nvt"

This doesn't cause an error within the package, but will cause the simulation to fail. We should consider ways to prevent this from happening. Maybe prevent the user from setting the ensemble outside of initializing the moves object.

rmatsum836 · 2020-04-16T17:48:42Z

Just realized I need to account for the restricted option in probability swap as well.

rmatsum836 · 2020-04-16T21:34:20Z

I think the PR is ready to be reviewed, I'm happy where it's at.

rsdefever

A few small picks, then LGTM.

(1) If I have a restricted insertion already added, then add another one, it appears that the first restricted insertion is replaced. Can we throw a warning?
(2) See comments about other errors and docstrings.

Once those are resolved I will merge! LGTM today!

rsdefever · 2020-04-17T18:03:07Z

mosdef_cassandra/core/moves.py

+        restricted_type : list
+            list of restricted insertion types, with one species per element.
+        restricted_value : list
+            list of restricted insertion values, with one species per element.


I think these could be more clear; i.e. WRT the nesting of lists. One list per box and then one list per species.

rsdefever · 2020-04-17T18:06:30Z

mosdef_cassandra/core/moves.py

+        for box in restricted_type:
+            if len(box) != len(species_topologies):
+                raise ValueError(
+                    "Length of 'species' and "
+                    " length of species list in 'restricted_type'"
+                    " must match."
+                )
+        for box in restricted_value:
+            if len(box) != len(species_topologies):
+                raise ValueError(
+                    "Length of 'species' and "
+                    " length of species list in 'restricted_value'"
+                    " must match."
+                )


Perhaps these error messages can be made more verbose. If I do this:

species_list = [methane] moves = mc.Moves('gcmc', species_list) moves.add_restricted_insertions(species_list, ['sphere'], [5.0])

The error message I get is:

ValueError: Length of 'species' and length of species list in 'restricted_type' must match.

I'm not sure that points me towards fixing my problem.

rmatsum836 added 3 commits April 1, 2020 14:23

Add initial restriction insertion attribute

f5e4b9b

Restructrue restriction_insert setter

4e58a88

Write out restricted insertions

a53da2c

rmatsum836 added the WIP label Apr 1, 2020

rsdefever added this to the Version 0.2.0 release milestone Apr 1, 2020

rmatsum836 added 7 commits April 2, 2020 09:12

Check valid ensemble with restricted insert, add printing

8cf5a58

Fix logic to support restrictions for 1 and 2 boxes

9b1a2eb

Add restricted as a probability insertin

578a0c8

Add basic moves tests

fbf9f72

Add writers tests for restricted insertions

956995e

Refactor restricted insertions as lists

b5ba8d6

Fix bugs so writer tests pass

95efc00

rmatsum836 added 4 commits April 9, 2020 12:37

Rewrite restricted insertions to support multiple boxes and species

c741b0f

parametrize resricted insertion test functions

decb013

Fix restriction tests in test_writers

500ed06

Add tests to ensure ValueError when invalid restricted insertion passed

469a712

rsdefever requested changes Apr 13, 2020

View reviewed changes

rsdefever removed the WIP label Apr 13, 2020

rmatsum836 added 3 commits April 15, 2020 09:45

Fix conversion from nm to a and add example

c941c4f

Remove chemical potential from gemc and reformat with black

b2b1099

Add docstring and reformat get_box_info in inp_functions

3edf6ab

rmatsum836 added 3 commits April 15, 2020 11:41

Refactor restricted insertions into a method

bf3d6c2

Fix error messages

0113cda

Add gcmc_restriction to init file

fc1adae

rmatsum836 added 2 commits April 16, 2020 09:30

Add restricted insertion tests designed to fail

610202c

Remove old implementation of restricted insertion

7cb4640

Revert restricted insertions to being passed in as angstroms

264e0eb

rsdefever mentioned this pull request Apr 16, 2020

Prevent ensemble from being edited in Moves object once initialized #50

Closed

Add restricted option for probability swap

2db7490

rsdefever requested changes Apr 17, 2020

View reviewed changes

Address Ryan's comments

9181fad

rsdefever merged commit 335a458 into MaginnGroup:master Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add restricted insertions attribute for `moves` #47

Add restricted insertions attribute for `moves` #47

rmatsum836 commented Apr 1, 2020 •

edited

Loading

rsdefever commented Apr 1, 2020

rmatsum836 commented Apr 8, 2020

rmatsum836 commented Apr 9, 2020

rmatsum836 commented Apr 13, 2020

rsdefever left a comment •

edited

Loading

rsdefever Apr 13, 2020

rsdefever Apr 13, 2020

rsdefever Apr 13, 2020

rsdefever Apr 13, 2020

rsdefever Apr 13, 2020

rsdefever Apr 13, 2020

rsdefever Apr 13, 2020

rsdefever Apr 13, 2020

rmatsum836 commented Apr 13, 2020

rsdefever commented Apr 13, 2020

rmatsum836 commented Apr 14, 2020

rsdefever commented Apr 14, 2020

rmatsum836 commented Apr 14, 2020

rmatsum836 commented Apr 15, 2020

rsdefever commented Apr 16, 2020

codecov-io commented Apr 16, 2020

rmatsum836 commented Apr 16, 2020

rmatsum836 commented Apr 16, 2020

rmatsum836 commented Apr 16, 2020

rsdefever left a comment

rsdefever Apr 17, 2020

rsdefever Apr 17, 2020

Add restricted insertions attribute for moves #47

Add restricted insertions attribute for moves #47

Conversation

rmatsum836 commented Apr 1, 2020 • edited Loading

rsdefever commented Apr 1, 2020

rmatsum836 commented Apr 8, 2020

rmatsum836 commented Apr 9, 2020

rmatsum836 commented Apr 13, 2020

rsdefever left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmatsum836 commented Apr 13, 2020

rsdefever commented Apr 13, 2020

rmatsum836 commented Apr 14, 2020

rsdefever commented Apr 14, 2020

rmatsum836 commented Apr 14, 2020

rmatsum836 commented Apr 15, 2020

rsdefever commented Apr 16, 2020

codecov-io commented Apr 16, 2020

Codecov Report

rmatsum836 commented Apr 16, 2020

rmatsum836 commented Apr 16, 2020

rmatsum836 commented Apr 16, 2020

rsdefever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add restricted insertions attribute for `moves` #47

Add restricted insertions attribute for `moves` #47

rmatsum836 commented Apr 1, 2020 •

edited

Loading

rsdefever left a comment •

edited

Loading