Added functionality and some fixes #1

hejamu · 2021-04-05T10:42:20Z

Hi all,

I use this modified version of flamel in my research. Maybe this is interesting for others.

Add missing unit conversion in pickle output

hejamu · 2021-04-05T10:47:30Z

I'll take the liberty of tagging @orbeckst and @dotsdl, since the original developer is no longer active in this field?

orbeckst · 2021-04-05T16:28:40Z

Sure, thanks for the contribution, @hejamu !

Please ping me when you need a review.

Generally speaking, smaller/focused PRs are easier to handle than big ones.

hejamu · 2021-04-05T17:42:51Z

Thanks @orbeckst.

Yeah, it's a bit of a mess with my commits... I didn't think about contributing my changes back when I started it... I guess I will just open the PR to initiate the discussion about it. The majority of the changes are just small additions and documentation.

orbeckst

@hejamu I had a few minutes to look over your PR. I am not an expert on flamel but apparently I am the only person looking at things at the moment so here we go.

The docs changes all look very good.

New output plugins are also good. I am happy to see that the plugin system works.

My primary concern is with code duplication, including how constants are sprinkled throughout the code. It would help a lot if you could come up with a way to collect all unit-conversion related code (including constants) in a separate module that is just imported in the other modules.

Similarly, at first glance there seems duplicated code in the plugins. If that's not the case then let me know. Otherwise this should all go into a plugin base class or you derive your class from another class that already has these (but it's cleaner to have a common base class so that changes in one class do not affect the other).

orbeckst · 2021-05-04T19:21:48Z

output/pickle.py

+
+class Pickle:
+    name = 'pickle'
+    k_b = 8.3144621E-3


If we need constants then we should have them all in one place.

It would be possible to use scipy.constants since scipy is a dependency of alchemlyb anyway?

Yes, good idea.

(You'll still have to add it to flamel's setup.py to be explicit.)

Er, sorry... there's no setup.py. In fact there's no installation at all... see #3

orbeckst · 2021-05-04T19:23:56Z

output/simple.py

+        if args.unit == 'kT':
+            conversion = 1.0
+            args.unit = 'kT'
+        elif args.unit == 'kJ' or args.unit == 'kJ/mol':
+            conversion = t * self.k_b
+            args.unit = 'kJ/mol'
+        elif args.unit == 'kcal' or args.unit == 'kcal/mol':
+            conversion = 0.239006 * t * self.k_b
+            args.unit = 'kcal/mol'
+


duplication of conversion code

orbeckst · 2021-05-04T19:25:25Z

output/alchemical_analysis.py

+
+        if args.unit == 'kT':
+            conversion = 1.0
+            args.unit = 'kT'
+        elif args.unit == 'kJ' or args.unit == 'kJ/mol':
+            conversion = 1.0 * t * self.k_b
+            args.unit = 'kJ/mol'
+        elif args.unit == 'kcal' or args.unit == 'kcal/mol':
+            conversion = 0.239006 * t * self.k_b
+            args.unit = 'kcal/mol'
+


duplication of conversion code — can this be put into a separate, reusable function ?

(There's some discussion on units in alchemlyb alchemistry/alchemlyb#125 but it will be a while until that is ready so in the meantime, flanel should certainly provide a way for unit conversion.)

Putting this code and its duplicates in a separate function is definitely the best way. The linked discussion is very interesting though.

By all means, contribute to the discussion! Contributors are very welcome.

orbeckst · 2021-05-04T19:25:57Z

output/text.py

+
+class Text:
+    name = 'text'
+    k_b = 8.3144621E-3


keep constants in one place

orbeckst · 2021-05-04T19:26:53Z

output/text.py

+    def ls(cls, estimators):
+        """
+        Return a list of lambda values
+        :param estimators: Series
+            List of estimator plugins
+        :return:
+            The list of lambda values
+        """
+        ls = []
+        if estimators:
+            if estimators[0].needs_dhdls:
+                means = estimators[0].dhdls.mean(level=estimators[0].dhdls.index.names[1:])
+                ls = np.array(means.reset_index()[means.index.names[:]])
+            elif estimators[0].needs_u_nks:
+                means = estimators[0].u_nks.mean(level=estimators[0].u_nks.index.names[1:])
+                ls = np.array(means.reset_index()[means.index.names[:]])
+
+        return ls


same code as in Pickle plugin?

orbeckst · 2021-05-04T19:27:12Z

output/text.py

+        return l_types
+
+    @classmethod
+    def segments(cls, estimators):


duplicate code?

orbeckst · 2021-05-04T19:27:37Z

output/text.py

+        if args.unit == 'kT':
+            conversion = 1.0
+            args.unit = 'kT'
+        elif args.unit == 'kJ' or args.unit == 'kJ/mol':
+            conversion = t * self.k_b
+            args.unit = 'kJ/mol'
+        elif args.unit == 'kcal' or args.unit == 'kcal/mol':
+            conversion = 0.239006 * t * self.k_b
+            args.unit = 'kcal/mol'


duplicated code... with lots of hard-coded constants :-p

orbeckst · 2021-05-04T19:32:52Z

uncorrelate/statistical_inefficiency_dhdl.py

-            ind = np.array(ind, dtype=int)
-            dhdl_sum = dhdl_.dot(ind)
-            uncorrelated_dfs.append(alchemlyb.preprocessing.statistical_inefficiency(df, dhdl_sum, lower, conservative=False))
+            uncorrelated_dfs.append(alchemlyb.preprocessing.statistical_inefficiency(df, lower=lower))


Set explicitly conservative=True.

orbeckst · 2021-05-04T19:33:14Z

uncorrelate/statistical_inefficiency_dhdl.py

-            ind = np.array(l, dtype=bool)
-            ind = np.array(ind, dtype=int)
-            dhdl_sum = dhdl_.dot(ind)


What's the rationale for not using dhdl_sum?

I can't recall why this change was made.... I will have to look into this.

This was a temporary change due to alchemistry/alchemlyb#166. We identified the cause just now. I reverted the change.

hejamu · 2021-05-04T19:49:50Z

@orbeckst Thanks for looking into this so thoroughly. The units really are a mess... I hope to get to the changes soon :)

orbeckst · 2021-07-30T18:49:08Z

@hejamu are you still interested in finalizing the PR?

About constants: use scipy.constants (alchemlyb switched to using it, too).

Main changes: - make the `energy_unit` attribute propagate through the `uncorrelate` plugins. - use alchemlybs new conversion feature to facilitate easy unit conversion. - Removed `text` output plugin. This was just a file version of the `alchemical-analysis` output. Moved into the `alchemical-analysis` plugin

hejamu · 2021-09-21T16:29:04Z

Sorry, this was low priority for me in the last months. I think these change are a step in the right direction.

orbeckst · 2021-09-22T17:25:18Z

Overall I like the code clean-up and how much @xiki-tempula 's new units system improved the code here!

In the absence of any tests (see #9 ) I am going to just say "ok". My feeling is that adding CI with tests (#9) is the next important milestone to give flamel a chance to grow and gain a user base.

orbeckst · 2021-09-22T17:26:12Z

@xiki-tempula @harlor do you want to have a look at this PR before a merge?

orbeckst · 2022-11-06T22:08:02Z

I manually merged this branch into master in 91d1cc4 so the code is in the retroactive 0.2.0.

I'll also merge into develop so that the branch shows up merged but we are not using develop anymore.

schlaicha and others added 12 commits November 6, 2020 10:59

Fix broken call to alchemlyb

d627c39

Some spelling corrections and additions

d9a472e

Added several alchemical-analysis cli features.

801e057

Added several alchemical-analysis cli features.

e081659

Reflect features in README

d18aefa

Fixed kcal calculation error.

761cf82

Add information about the pickle file.

fc7b500

Fix closing backtick

5c1070b

Fixed unit conversion error

40578e5

Add missing unit conversion in pickle output

738dd88

Added support for recursive file search

e3c916c

Merge pull request #1 from schlaicha/feature

c7cbfd1

Add missing unit conversion in pickle output

hejamu marked this pull request as ready for review April 5, 2021 17:42

orbeckst requested changes May 4, 2021

View reviewed changes

reverts changes in d627c39

33ad6c5

orbeckst approved these changes Sep 22, 2021

View reviewed changes

orbeckst added the enhancement New feature or request label Sep 22, 2021

orbeckst self-assigned this Sep 23, 2021

orbeckst mentioned this pull request Nov 6, 2022

Convert flamel to a simple CLI #10

Merged

orbeckst merged commit 853ae2c into alchemistry:develop Nov 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added functionality and some fixes #1

Added functionality and some fixes #1

hejamu commented Apr 5, 2021

hejamu commented Apr 5, 2021

orbeckst commented Apr 5, 2021

hejamu commented Apr 5, 2021

orbeckst left a comment

orbeckst May 4, 2021

hejamu May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

hejamu May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

orbeckst May 4, 2021

hejamu Sep 21, 2021

orbeckst May 4, 2021

hejamu Sep 21, 2021

hejamu Sep 22, 2021

hejamu commented May 4, 2021

orbeckst commented Jul 30, 2021

hejamu commented Sep 21, 2021

orbeckst commented Sep 22, 2021

orbeckst commented Sep 22, 2021

orbeckst commented Nov 6, 2022

Added functionality and some fixes #1

Added functionality and some fixes #1

Conversation

hejamu commented Apr 5, 2021

hejamu commented Apr 5, 2021

orbeckst commented Apr 5, 2021

hejamu commented Apr 5, 2021

orbeckst left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hejamu commented May 4, 2021

orbeckst commented Jul 30, 2021

hejamu commented Sep 21, 2021

orbeckst commented Sep 22, 2021

orbeckst commented Sep 22, 2021

orbeckst commented Nov 6, 2022