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 request: update Ensemble's ancil #192

Open
hombit opened this issue Aug 31, 2023 · 2 comments
Open

Feature request: update Ensemble's ancil #192

hombit opened this issue Aug 31, 2023 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@hombit
Copy link
Member

hombit commented Aug 31, 2023

Current Ensemble.set_ancil rewrites the ancil completely and doesn't preserve what is already there. So this fails:

ens.set_ancil(dict(a=np.linspace(0, 1, n)))
ens.set_ancil(dict(b=np.linspace(1, 2, n)))
assert 'a' in ens.ancil

If I'd like to update it I could do something like this (in modern Python):

ens.set_ancil(ens.ancil | dict(b=np.linspace(1, 2, n)))

It would be fine to have update_ancil method of change the behavior of the current set_anciil.

NB. Actually, the current implementation of the ancil property also allows me to do this and change the underlying ens._encil in the way I want, is it intentional?

ens.ancil['b'] = "hello world"

I would suggest to change the implementation to produce a shallow copy of the attribute so users do not have an access to the original data (but still could accidentally change content of the underlying numpy arrays):

@property
def ancil(self):
    """Return the ancillary data dictionary"""
    return self._ancil.copy()
@aimalz aimalz added bug Something isn't working help wanted Extra attention is needed labels Aug 31, 2023
@aimalz
Copy link
Collaborator

aimalz commented Aug 31, 2023

Fixing this would also largely address LSSTDESC/rail_base#39, specifically the bits about copying over an ensemble's ancil when converting between parameterizations and the nonintuitive behavior of set_ancil and add_ancil, versus changing the value of the underlying _ancil attribute directly with =. Also, I'm happy to close that issue if folks agree it's appropriate to expand the scope of this one to add support for including ancil as a keyword argument upon instantiating a qp.Ensemble object, however, I'm probably not the best sole assignee due to unfamiliarity with the implications of the proposed design versus naive alternatives.

@eacharles eacharles added enhancement New feature or request and removed bug Something isn't working labels Sep 11, 2023
@eacharles
Copy link
Collaborator

eacharles commented Sep 11, 2023

So, the add_to_ancil function does update() with the addition of shape checking. I suppose it could be called update_ancil, but I think the behavior should be pretty clear from the name.

And the set_ancil function set's the entire dictionary, which is pretty standard behavior for a function called 'set_xxx'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants