-
Notifications
You must be signed in to change notification settings - Fork 26
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/flatten asktell dict values #1409
Feature/flatten asktell dict values #1409
Conversation
Bumps [supercharge/redis-github-action](https://github.com/supercharge/redis-github-action) from 1.7.0 to 1.8.0. - [Release notes](https://github.com/supercharge/redis-github-action/releases) - [Changelog](https://github.com/supercharge/redis-github-action/blob/main/CHANGELOG.md) - [Commits](supercharge/redis-github-action@1.7.0...1.8.0) --- updated-dependencies: - dependency-name: supercharge/redis-github-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Fixing broken link
…rator class can set gen_specs.user via kwargs
…ent from input list of dicts
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## experimental/jlnav_plus_shuds_asktell #1409 +/- ##
==========================================================================
- Coverage 90.56% 77.17% -13.39%
==========================================================================
Files 88 94 +6
Lines 8055 8425 +370
Branches 1442 1503 +61
==========================================================================
- Hits 7295 6502 -793
- Misses 570 1698 +1128
- Partials 190 225 +35 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss the theory, but the implementation is partial, and would need to be made consistent.
bump pydantic versions, really trying to resolve warnings
Various fixes to run the proxystore test on extra-ci
…evelop/supercharge/redis-github-action-1.8.0 Bump supercharge/redis-github-action from 1.7.0 to 1.8.0
… classes, fix aposmm unit test
…ffix, instead of just checking if last char is digit. better decide output numpy array type for strings
Bumps [globus-compute-sdk](https://github.com/globus/globus-compute) from 2.26.0 to 2.27.0. - [Release notes](https://github.com/globus/globus-compute/releases) - [Changelog](https://github.com/globus/globus-compute/blob/main/docs/changelog.rst) - [Commits](globus/globus-compute@2.26.0...2.27.0) --- updated-dependencies: - dependency-name: globus-compute-sdk dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…r keys. e.g. {"co2": 12}
…us-compute-sdk-2.27.0 Bump globus-compute-sdk from 2.26.0 to 2.27.0
Bumps [crate-ci/typos](https://github.com/crate-ci/typos) from 1.23.6 to 1.23.7. - [Release notes](https://github.com/crate-ci/typos/releases) - [Changelog](https://github.com/crate-ci/typos/blob/master/CHANGELOG.md) - [Commits](crate-ci/typos@v1.23.6...v1.23.7) --- updated-dependencies: - dependency-name: crate-ci/typos dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
…evelop/crate-ci/typos-1.23.7 Bump crate-ci/typos from 1.23.6 to 1.23.7
Bumps [crate-ci/typos](https://github.com/crate-ci/typos) from 1.23.7 to 1.24.1. - [Release notes](https://github.com/crate-ci/typos/releases) - [Changelog](https://github.com/crate-ci/typos/blob/master/CHANGELOG.md) - [Commits](crate-ci/typos@v1.23.7...v1.24.1) --- updated-dependencies: - dependency-name: crate-ci/typos dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
This is the test data I'm using. I suggest it for unit tests because its awkward. dtype = [('a', 'i4'), ('x', 'f4', (3,)), ('y', 'f4', (1,)), ('z', 'f4', (12,)), ('greeting', 'U10'), ('co2', 'f8')]
H = np.zeros(2, dtype=dtype)
H[0] = (1, [1.1, 2.2, 3.3],[10.1],[1,2,3,4,5,6,7,8,9,10,11,12], 'hello','1.23')
H[1] = (2, [4.4, 5.5, 6.6],[11.1],[51,52,53,54,55,56,57,58,59,60,61,62], 'goodbye','2.23')
list_dicts = np_to_list_dicts(H)
npp = list_dicts_to_np(list_dicts)
for field in H.dtype.names:
print(f"Comparing {field}: {H[field]} {npp[field]}")
if isinstance(H[field], np.ndarray):
assert np.array_equal(H[field], npp[field]), f"Mismatch found in field {field}"
elif isinstance(H[field], str) and isinstance(npp[field], str):
assert H[field] == npp[field], f"Mismatch found in field {field}"
elif np.isscalar(H[field]) and np.isscalar(npp[field]):
assert np.isclose(H[field], npp[field]), f"Mismatch found in field {field}"
else:
raise TypeError(f"Unhandled or mismatched types in field {field}: {type(H[field])} vs {type(npp[field])}") Get failures still. The failure for y I think is because the information that it was a 1D array is lost when convert to dictionary. So this type of thing is why may need to reference original data for back/forth conversion. I'm not sure this can be done as standalone functions. The string also fails, maybe you also lose the size 'U10' when convert to dictionary, so when it comes back, it is taking length of first string entry - but they are different lengths ('hello', 'goodbye') |
…evelop/crate-ci/typos-1.24.1 Bump crate-ci/typos from 1.23.7 to 1.24.1
Going to implement your test data into the unit test. Yeah, I'll see what can be done about For the string, yes; currently taking the length of a string in the first entry; but going through the whole dict and finding the largest string also feels wrong. When I've placed strings into the History into the past I try to make sure they're all the same length, or very close. Maybe I could do instead: But yeah; without the original knowledge around, we'll lose the original string datatype. |
Starting to lean towards a And as we've discussed it would be best not to force users of external gens to specify dtypes...? |
… for np_to_list_dicts to unpack length-1 arrays/lists, into scalars
…scovery" routine. formatting
…cts_to_np passes through input as-is if its not a list (already numpy, no conversion necessary. _to_array did this previously)
… but its only necessary within the ask()
…cribing the names of the fields, decides what fields are passed in, but their "actual datatypes" come from the sim / sim_specs
libensemble/generators.py
Outdated
self.gen_specs["user"] = kwargs | ||
if not persis_info: | ||
self.persis_info = add_unique_random_streams({}, 4, seed=4321)[1] | ||
self.persis_info["nworkers"] = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats going on here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kwargs line is similar to #1307 (comment)
for persis_info
, if libE ask/tell generators maintain the H, persis_info, gen_specs, libE_info, **kwargs
signature, my thinking is users may prefer not needing to populate/maintain persis_info.
Its the circumstance where
aposmm = APOSMM(
ub=1,
lb=0,
)
is probably preferred to:
aposmm = APOSMM(
None,
{},
{
"user": {"ub": 1, "lb": 0},
},
{}
)
libensemble/gen_classes/sampling.py
Outdated
self._get_user_params(self.gen_specs["user"]) | ||
self.gen_specs["out"] = [("x", float, (self.n,))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this. We are trying to represent an external gen here. Most external gens are not going to have this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point; I don't think gen_specs["out"]
is necessary for this specific class
libensemble/gen_classes/sampling.py
Outdated
self._get_user_params(self.gen_specs["user"]) | ||
self.gen_specs["out"] = [("x", float, (self.n,))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. This would overwrite any gen_specs['out'] supplied, but I can see that declaring the input/output types should probably come with the gen. Although its possible it may support more than one type or size of float or dimensionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, declaring input/output types is often implied by libE's interface to be something you can do; and something we should wholeheartedly support. but for UniformSample, for example, does it create anything other than x
s ?
I was thinking that it seems it doesn't matter if you decide to uniformly sample thetas instead:
gen_specs["out"] = ["theta", float, (3,)]
the fields are often nonetheless hardcoded like:
H_o["x"] = ...
Maybe that's just because its a very simple gen. But this is also true for our other gens; you really can't customize gen_specs["out"]
except for sizes.
So might as well hardcode the gen's fields, I guess, and make sure its clear to a user that its really the size thats customizable.
Though I guess if fields really were configurable, we'd have a gen that resembles:
for field in gen_specs["out"]:
if field == "theta":
H["theta"] = do_experiment_with_theta()
else:
H["x"] = do_without_theta()
libensemble/generators.py
Outdated
@@ -104,7 +117,7 @@ def ask(self, num_points: Optional[int] = 0) -> List[dict]: | |||
|
|||
def tell(self, results: List[dict]) -> None: | |||
"""Send the results of evaluations to the generator.""" | |||
self.tell_numpy(list_dicts_to_np(results)) | |||
self.tell_numpy(list_dicts_to_np(results)) # OH, we need the union of sim_specs.out and gen_specs.out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If using externally may not even have sim_specs, or gen_specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I think we briefly discussed on Tuesday that we have to do the conversion anyway; and thankfully libE can handle receiving numpy arrays with dtypes it previously was unaware of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the external interface, I would think each gen must need to say name/type for dictionaries entries (in VOCS). So user knows what they must supply. But they only allow scalars and something like sampling can take varying dimensions, what would the interface be. x0, x1, x2 ... Xn
…kers, use gen_specs.get("out") so if it isnt provided, the dtype discovery process commences
…stead of it deciding it for itself internally
345aea3
into
experimental/jlnav_plus_shuds_asktell
LibensembleGenerator
parent class