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

Use Sage 2.2.0 RC1 as default #307

Merged
merged 5 commits into from
Apr 22, 2024
Merged

Use Sage 2.2.0 RC1 as default #307

merged 5 commits into from
Apr 22, 2024

Conversation

mattwthompson
Copy link
Member

Copy link
Contributor

@jthorton jthorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mattwthompson it would be great to make this change!

@mattwthompson mattwthompson marked this pull request as ready for review February 19, 2024 15:38
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Merging #307 (f1ba30b) into main (f7cd2a6) will increase coverage by 0.05%.
The diff coverage is n/a.

Additional details and impacted files

@mattwthompson
Copy link
Member Author

No idea why the OpenEye integration tests are failing ...

@jthorton
Copy link
Contributor

Oh no looks like some strange interaction with fitting on top of this force field. We did see an error like this before but that was fixed by using the new geometric #243.

@mattwthompson
Copy link
Member Author

Any other ideas where to start? The main thing that stands out to me is the toolkit difference, but I doubt that either

  • Omega would give an ethane conformer so bad it can't be optimized or
  • ELF10 gives substantively different charges such that the optimization should crash

@mattwthompson
Copy link
Member Author

I don't know how to isolate this behavior locally, and there have been some other quiet issues bumping OpenEye Toolkits from 2022 to 2023, so I'm just going to constrain down to what we know worked before

@j-wags
Copy link
Member

j-wags commented Feb 28, 2024

@jthorton Any other ideas? Matt and I just looked at this and are stumped. It looks like we're using the latest geometric too.

@mattwthompson
Copy link
Member Author

A more useful error:

/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/forcebalance/smirnoffio.py:422: InterchangeDeprecationWarning: The `handlers` attribute is deprecated. Use `collections` instead.
  if 'VirtualSites' in interchange.handlers:
Traceback (most recent call last):
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/bin/ForceBalance.py", line 45, in Run_ForceBalance
    optimizer.Run()
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/forcebalance/optimizer.py", line 322, in Run
    xk = self.OptTab[self.jobtype]()
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/forcebalance/optimizer.py", line 951, in NewtonRaphson
    return self.MainOptimizer(b_BFGS=0)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/forcebalance/optimizer.py", line 676, in MainOptimizer
    dx, dX_expect, bump = self.step(xk, data, trust)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/forcebalance/optimizer.py", line 929, in step
    Result = optimize.brent(search_fun,brack=(LOpt,LOpt*4),tol=self.search_tol,full_output=1)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/scipy/optimize/_optimize.py", line 2730, in brent
    res = _minimize_scalar_brent(func, brack, args, **options)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/scipy/optimize/_optimize.py", line 2767, in _minimize_scalar_brent
    brent.optimize()
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/scipy/optimize/_optimize.py", line 2537, in optimize
    xa, xb, xc, fa, fb, fc, funcalls = self.get_bracket_info()
                                       ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/scipy/optimize/_optimize.py", line 2506, in get_bracket_info
    xa, xb, xc, fa, fb, fc, funcalls = bracket(func, xa=brack[0],
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mattthompson/micromamba/envs/bespokefit-test/lib/python3.11/site-packages/scipy/optimize/_optimize.py", line 3136, in bracket
    raise e
scipy.optimize._optimize.BracketError: The algorithm terminated without finding a valid bracket. Consider trying different initial points.

@mattwthompson mattwthompson changed the title Use Sage 2.1.0 as default Use Sage 2.2.0 RC1 as default Mar 11, 2024
@j-wags
Copy link
Member

j-wags commented Mar 11, 2024

I don't think we should merge this in a form where a RC is the default FF. It's fine if we use it as part of the debugging process but we shouldn't put an RC into production.

@mattwthompson
Copy link
Member Author

Right, but this should be ready to go on release

@mattwthompson mattwthompson marked this pull request as draft March 11, 2024 17:58
@jthorton
Copy link
Contributor

Well this is very confusing as the new RC passes all the tests.

@mattwthompson mattwthompson marked this pull request as ready for review March 27, 2024 16:51
@mattwthompson
Copy link
Member Author

Won't work until conda-forge/openff-forcefields-feedstock#25 is through

@j-wags
Copy link
Member

j-wags commented Apr 19, 2024

The new FF package should be available now. Fixed a little typo and kicked CI.

@mattwthompson
Copy link
Member Author

This has one approving review, which is fairly old but in the spirit of its current state. Everything is green, so I'm merging

@mattwthompson mattwthompson merged commit 6602daf into main Apr 22, 2024
7 checks passed
@mattwthompson mattwthompson deleted the openff-2.1.0 branch April 22, 2024 17:35
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.

3 participants