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

Feature/decorators simplify examples #1175

Merged
merged 44 commits into from
Jan 24, 2024

Conversation

jlnav
Copy link
Member

@jlnav jlnav commented Nov 22, 2023

Tentative refactoring of a handful of mostly functionality tests based on decorated example functions

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (4632fa4) 72.20% compared to head (90b584f) 72.36%.
Report is 3 commits behind head on develop.

Files Patch % Lines
libensemble/specs.py 42.85% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1175      +/-   ##
===========================================
+ Coverage    72.20%   72.36%   +0.16%     
===========================================
  Files           93       93              
  Lines         8252     8275      +23     
  Branches      1467     1487      +20     
===========================================
+ Hits          5958     5988      +30     
+ Misses        2042     2036       -6     
+ Partials       252      251       -1     

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

Base automatically changed from feature/ufunc_field_decorators to develop December 14, 2023 21:15
@jlnav jlnav requested review from shuds13 and jmlarson1 December 14, 2023 22:15
@jmlarson1
Copy link
Member

@jlnav This greatly simplifies our many tests, which is nice. But I'm concerned it hides the necessary input/output relationships between the sims and gens. It is nice to have the decorators available, but we should think about how much we want to use them.

@jlnav
Copy link
Member Author

jlnav commented Dec 15, 2023

@jlnav This greatly simplifies our many tests, which is nice. But I'm concerned it hides the necessary input/output relationships between the sims and gens. It is nice to have the decorators available, but we should think about how much we want to use them.

Good point. We know for a given sim and gen that they're compatible if their outputs and inputs correspond.

I just realized though, suppose a user tries to combine two user functions that are currently incompatible, their ins and outs don't match up. Changing the ins and outs within sim_specs and gen_specs doesn't solve their incompatibility! In fact, adjusting the ins/outs may cause the specs to pass validation, but errors would occur within the user function anyway.

I'll probably undo the regression test changes, since those are more user-examples.

@shuds13 shuds13 mentioned this pull request Dec 15, 2023
20 tasks
@@ -528,7 +530,9 @@ def persistent_uniform(_, persis_info, gen_specs, libE_info):

def decorator(func):
setattr(func, "persis_in", fields)
func.__doc__ = f"\n **Persistent Input Fields:** ``{func.persis_inputs}``\n" + func.__doc__
if not func.__doc__:
func.__doc__ = ""
Copy link
Member

Choose a reason for hiding this comment

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

Is there no test using persis_in? (Codecov says it's not covered)

Copy link
Member Author

Choose a reason for hiding this comment

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

These uncovered lines refer to user functions without docstrings, something that thankfully we have very few of! I'll see what I can do

@@ -63,7 +63,6 @@
# State the generating function, its arguments, output, and necessary parameters.
gen_specs = {
"gen_f": gen_f, # Generator function
"in": [], # Generator input
Copy link
Member

Choose a reason for hiding this comment

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

Is removing this line necessary. I would rather not change it.

@@ -41,7 +41,6 @@

ensemble.gen_specs = GenSpecs(
gen_f=gen_f,
inputs=[], # No input when start persistent generator
Copy link
Member

Choose a reason for hiding this comment

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

as above

@jlnav jlnav merged commit ad34b33 into develop Jan 24, 2024
16 checks passed
@jlnav jlnav deleted the feature/decorators_simplify_examples branch January 24, 2024 21:18
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