Skip to content

Conversation

jlnav
Copy link
Member

@jlnav jlnav commented May 9, 2024

See checklist in #1307 (review)

@jlnav jlnav marked this pull request as ready for review May 16, 2024 20:06
Copy link

codecov bot commented May 16, 2024

Codecov Report

❌ Patch coverage is 92.09184% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.95%. Comparing base (ce582d2) to head (6b54991).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
libensemble/generators.py 85.05% 7 Missing and 6 partials ⚠️
libensemble/gen_classes/aposmm.py 88.40% 1 Missing and 7 partials ⚠️
libensemble/utils/runners.py 93.33% 3 Missing and 3 partials ⚠️
libensemble/utils/misc.py 96.87% 1 Missing and 2 partials ⚠️
libensemble/libE.py 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #1307       +/-   ##
============================================
+ Coverage    78.23%   89.95%   +11.72%     
============================================
  Files           76       79        +3     
  Lines         7561     7915      +354     
  Branches      1116     1176       +60     
============================================
+ Hits          5915     7120     +1205     
+ Misses        1447      615      -832     
+ Partials       199      180       -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

}

my_APOSMM = APOSMM(gen_specs)
my_APOSMM.setup()
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be separate to constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps you've seen already, but setup() sets attributes that can't be pickled, but need to be done anyway. So I believe a separate generator.setup() needs to exist.

But perhaps for outside-of-libE purposes, to save a line, this could resemble: my_APOSMM = APOSMM(gen_specs, setup=True).

@shuds13
Copy link
Member

shuds13 commented May 30, 2024

@jlnav

Currently this has two existing user functions refactored to ask/tell, and it is a breaking change for codes using those gens. I don't think this is what we want. So, for now, we could supply these as duplicates (keeping the original), or remove them. Alternative would be to refactor all appropriate gens to ask/tell, but that would hold this up, so I think it may be better to supply these two as a duplicate for now.

Also, would it be better for these ask/tell gens (rand sample and gpCAM) to use AskTellGenRunner and not use the wrapper gen_f (given a breaking change anyway).

Issues we need to address:

  • cleanup/resolve ask/tell refactoring - clear separation of gens - see branch Making new gpCAM gen class #1316
  • Alt. could be to make old imports use the ask/tell gens?
  • re-test runs with libE and Optimas that previously failed with threads
  • compare running with wrapper and ask/tell runner - compare results
    - results match. ask/tell runner about 10% slower than wrapper for randsample test.
  • CI passing
  • Parameters in wrapper were rearranged (???) - requires discussion (for now changing on Making new gpCAM gen class #1316) - deciding this is another reason why we may not want to replace the old gen_f's yet.
  • test_1d_asktell_gen.py is not 1d, its 2d - changing on Making new gpCAM gen class #1316
  • are we now getting reproducible/matching random numbers in APOSMM ask/tell test - check seeding (inc. when running via Optimas).
  • Need assert on minima found in aposmm test - if losing precision - why.
  • check comparison using diff alloc for APOSMM - including initial sample processing.

@jlnav
Copy link
Member Author

jlnav commented May 30, 2024

@jlnav

Currently this has two existing user functions refactored to ask/tell, and it is a breaking change for codes using those gens. I don't think this is what we want. So, for now, we could supply these as duplicates (keeping the original), or remove them. Alternative would be to refactor all appropriate gens to ask/tell, but that would hold this up, so I think it may be better to supply these two as a duplicate for now.

Sounds good. Maybe we could raise DeprecationWarnings?

Also, would it be better for these ask/tell gens (rand sample and gpCAM) to use AskTellGenRunner and not use the wrapper gen_f?

I prefer the AskTellGenRunner myself (I did develop it), but if all the gens move to my runner, then what would you want to do with your wrapper?

@shuds13
Copy link
Member

shuds13 commented May 30, 2024

I think there is also some opportunity for inheritence with the gpCAM class. And gp_cam_simple is more complicated than gp_cam_asktell. So, if anything the latter would be the base class, and renamed to avoid confusion. See #1316

@@ -441,6 +441,24 @@ def libE_mpi_worker(libE_comm, sim_specs, gen_specs, libE_specs):
# ==================== Local version ===============================


def _retrieve_generator(gen_specs):
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why this is needed. If gen is on a thread, should not need to be pickled.

Copy link
Member

@shuds13 shuds13 Jun 1, 2024

Choose a reason for hiding this comment

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

I've removed on my branch for now, so don't change here

@jlnav
Copy link
Member Author

jlnav commented Jun 5, 2024

Checking random sample when running without Optimas:

>>> import numpy as np
>>> normal = np.load("persistent_aposmm_nlopt_history_length=2005_evals=2000_workers=4.npy")
>>> asktell = np.load("persistent_aposmm_nlopt_asktell_history_length=2002_evals=2000_workers=3.npy")
>>> all([i in asktell["x"][:100] for i in normal["x"][:100]])
True

@jlnav
Copy link
Member Author

jlnav commented Jun 5, 2024

With Optimas, currently looks like random sample doesn't match, even when trying to account for seed

@jlnav
Copy link
Member Author

jlnav commented Jun 6, 2024

Precision fixed in the Optimas example: optimas-org/optimas@08a835b

@shuds13
Copy link
Member

shuds13 commented Jul 17, 2024

We may need to update gpCAM gens for latest gpCAM release. Make sure any changes made in gen_f is reflected here before pulling in.

  • Check gpCAM updates

@jlnav jlnav requested a review from shuds13 July 30, 2024 22:00
self.all_y = np.vstack((self.all_y, self.y_new))

if self.my_gp is None:
self.my_gp = GP(self.all_x, self.all_y, noise_variances=self.noise * np.ones(len(self.all_y)))
Copy link
Member

Choose a reason for hiding this comment

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

I need to update for new gpCAM interface

"""

def __init__(self, _, persis_info, gen_specs, libE_info=None) -> list:
# self.H = H
Copy link
Member

Choose a reason for hiding this comment

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

Remove I guess.

self.all_y = np.empty((0, 1))
np.random.seed(0)

def __init__(self, H, persis_info, gen_specs, libE_info=None):
Copy link
Member

Choose a reason for hiding this comment

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

I will put above _initialize_gpcAM

Copy link
Member

Choose a reason for hiding this comment

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

and make _initialize_gpcAM _initialize_gpCAM

jlnav and others added 30 commits July 16, 2025 16:36
…vious states, except they use VOCS instead of variables/objectives. Quick fix to the end of the manager routine to make num_gens_started=0; otherwise we run into the *technically already understood and dealt with problem* where the gen doesn't start again upon an additional libE()
…ging len(np.unique(H["gen_ended_time"])) != num_batches; it exceeds it
…ers .ask(15) instead of the default 4, re-add the gpcam gen
Experiments with VOCS in our gen classes
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