Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1601 +/- ##
==========================================
+ Coverage 86.82% 87.06% +0.23%
==========================================
Files 436 439 +3
Lines 57838 58995 +1157
Branches 5457 5568 +111
==========================================
+ Hits 50217 51363 +1146
- Misses 6186 6188 +2
- Partials 1435 1444 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Just use an "EM" Simulation
(For now) These should be changed to errors for futures
As suggested in their documentation
|
I think the new interface is coming together, here is a summary of the changes: New interfaceOn the constructor you can do: Or changing it after creating a simulation: When properties are
Added methodsThese public methods are added to the
DerivativesGetting the derivative of a property is now handled through a function, as opposed to a property, allowing us to (eventually) pass through multiplication vectors to the parametrizations.
Unless anyone has strong feelings otherwise, I'm in favor of keeping this private to keep the public API cleaner. I'll add more details in the future |
|
These are things that would be deprecated or removed with this PR. DeprecationsOn initialization
Attribute Assignment:
Attribute Access:
Removals
|
lheagy
left a comment
lheagy
left a comment
There was a problem hiding this comment.
Thanks for bringing this up again @jcapriot and sorry it took me so long to get back to this. I think this idea can work. I have just done a light pass to ask a few questions. Would you mind giving a demo again at an upcoming SimPEG meeting? It would be valuable to share this idea again with the group and get input from others. I think this could simplify things for users, but would appreciate chatting through implications for developers and people working to understand the code
|
|
||
| # simpeg problem and survey | ||
| prob = simulation.LinearSimulation(mesh, G=G, model_map=maps.IdentityMap()) | ||
| prob = simulation.LinearSimulation(G=G, model_map=maps.IdentityMap()) |
There was a problem hiding this comment.
Is this change intended to be in this PR? I am not sure it is in this scope? or maybe I am missing something
| noise_floor = 0.0 | ||
|
|
||
| prob1 = simulation.LinearSimulation(mesh, G=G, model_map=wires.m1) | ||
| prob1 = simulation.LinearSimulation(G=G, model_map=wires.m1) |
There was a problem hiding this comment.
similarly here, are these meant to be a part of this pr? it seems a bit different in scope?
| val = recip.default | ||
| # If it wasn't None, try to invert it. | ||
| if val is not None: | ||
| val = 1 / val |
There was a problem hiding this comment.
do we need a check that this is a scalar (or at least isotropic)? This wouldn't work if anisotropic
There was a problem hiding this comment.
Can update it to be anisotropic aware. At most it would be a block operation of inverting 3x3 matrices.
| self.fset(obj, value) | ||
| else: | ||
| self._fset(obj, value) | ||
| obj._remove_parametrization(self.name) |
There was a problem hiding this comment.
if I am following correctly, this would be hit if someone swapped out a mapping with a value or another mapping, is that right? If so, we might want to throw a warning? Changing something from an invertible parameter to not has some potentially involved implications
| warnings.warn( | ||
| f"Setting both `{prop1.name}` and `{prop2.name}` to None for `{type(self).__name__}`'s required " | ||
| f"physical properties is deprecated behavior. This message will be changed to an error in simpeg " | ||
| f"version X.X", | ||
| FutureWarning, | ||
| stacklevel=3, | ||
| # f"`{type(self).__name__}` requires one of `{prop1.name}` or `{prop2.name}`" | ||
| ) |
There was a problem hiding this comment.
A question on warning / error handling, will the user first see that they need to change sigmaMap=mapping to sigma=mapping? If not, this warning might be confusing
| warnings.warn( | ||
| f"Setting `{cls_name}.{old_map}` directly is deprecated. Instead register a parametrization with `{cls_name}.parametrize('{new_name}')`", | ||
| FutureWarning, |
There was a problem hiding this comment.
Can we also add that the user can set the mapping in the class instantiation via prop=mapping. I think that is the behaviour that we will want to encourage (I suspect most people will find the transition from sigmaMap=mapping --> sigma=mapping much easier than going through the class.parameterize ..... approach
santisoler
left a comment
santisoler
left a comment
There was a problem hiding this comment.
I like the ideas in this PR! I think I mentioned this several times, but I would also like to merge the physical property and their mapping into a single argument in simulations.
I also like how the PR groups together all things related to a physical property into a single object (instead of having sigma, sigmaMap, sigmaDeriv, for example).
Another thing I quite like is how easy it would be to know the shape of the expected model directly from the parametrization list. This would be a great feature to have when using SimPEG simulations in the new inversion framework, where the DataMisfit tries to get the n_params property from the simulation itself. Currently that's a bit of a challenge that varies across different simulations.
I'm leaving a few comments below so we can move forward with this.
Error messages while setting physical property as mapping
I would improve the error message when trying to set a physical property as a mapping. For example, if I run simulation.sigma = conductivity_mapping I get the following error:
TypeError: '`Simulation3DNodal.sigma`' must be array_like with data type of <class 'float'>, got <class 'simpeg.maps._injection.InjectActiveCells'>
Maybe we could make this error clearer, and point users on using the new simulation.parametrize(sigma=conductivity_model) interface.
Making physical_properties and invertible_properties properties?
WOuld it be possible to make the physical_properties and invertible_properties as (Python) properties of the class, so we could access them through:
simulation.physical_propertiesInstead of having to call them (as methods)?
Updating deprecation admonitions
I noticed a few admonitions in docs warning users about deprecations of the old properties. Most of them have old versions of SimPEG. I would update them to the next release: v0.26.0.
Connecting exclusive physical properties?
I think I briefly mentioned this idea in the last meeting, but I'll try to expand more on it here.
I was wondering if it would be best to group exclusive physical properties (exclusive in the sense of XOR, i.e. one can either pass one XOR the other) together in a single PhysicalProperty.
Just brainstorming, but something like:
sigma = props.PhysicalProperty(
"Electrical conductivity (S/m)",
reciprocal=props.PhysicalProperty( "Electrical resistivity (Ohm m)"),
shape=[(), (1,), ("mesh.n_cells",)],
)The HasModel can inject the two properties and the setters for each one of them. This way we could avoid having to handle the two of them separately in a few different places.
I don't intend to change the public interface, just refactor some of the internals so two reciprocal physical properties are naturally connected to each other.
Is it a bad idea?
| return newcls | ||
|
|
||
|
|
||
| class ParametrizationList: |
There was a problem hiding this comment.
On this note, I was initially going to suggest to make this a child of collections.UserDict to inherit the interface of dictionaries. But maybe it will require even more work to ensure that the list is "read only".
| if key in self.__slots__: | ||
| super().__setattr__(key, value) | ||
| elif key in self._fields: | ||
| raise AttributeError(f"'Cannot set attribute '{key}'") from None |
There was a problem hiding this comment.
We could potentially extend this error message, explaining that the ParametrizationList is meant to be read-only and we cannot modify the existing attribute.
|
|
||
| """ | ||
| for attr, parametrization in kwargs.items(): | ||
| if not isinstance(parametrization, maps.IdentityMap): |
There was a problem hiding this comment.
I'm wondering if situations like these ones might be a good opportunity to user typing.Protocols decorated with the @runtime_checkable.
I understand that we are currently using the IdentityMap as the base class for every map (something that it's weird, since all the other maps are not the identity one). So we could potentially make the distinction here that we expect the parametrization to be a mapping but not necessarily an IdentityMap.
There was a problem hiding this comment.
I do agree it would be good to add them as a protocol check. I put the initials of it here, but it would take several changes in ‘maps.py’ to properly support it.
| @sigma.setter | ||
| def sigma(self, value): | ||
| if value is not None: | ||
| prop = type(self).sigma | ||
| value = validate_ndarray_with_shape( | ||
| f"`{type(self).__name__}.sigma`", | ||
| value, | ||
| shape=[(), (1,), (self.mesh.n_cells,)], | ||
| dtype=float, | ||
| ) | ||
| setattr(self, prop.private_name, value) | ||
|
|
||
| for mat in self._clear_on_sigma_update: | ||
| if hasattr(self, mat): | ||
| delattr(self, mat) | ||
|
|
||
| @sigma.deleter | ||
| def sigma(self): | ||
| prop = type(self).sigma | ||
| if hasattr(self, prop.private_name): | ||
| delattr(self, prop.private_name) | ||
|
|
||
| for mat in self._clear_on_sigma_update: | ||
| if hasattr(self, mat): | ||
| delattr(self, mat) | ||
|
|
||
| @rho.setter | ||
| def rho(self, value): | ||
| if value is not None: | ||
| prop = type(self).rho | ||
| value = validate_ndarray_with_shape( | ||
| f"`{type(self).__name__}.rho`", | ||
| value, | ||
| shape=[(), (1,), (self.mesh.n_cells,)], | ||
| dtype=float, | ||
| ) | ||
| setattr(self, prop.private_name, value) | ||
|
|
||
| for mat in self._clear_on_rho_update: | ||
| if hasattr(self, mat): | ||
| delattr(self, mat) | ||
|
|
||
| @rho.deleter | ||
| def rho(self): | ||
| prop = type(self).rho | ||
| if hasattr(self, prop.private_name): | ||
| delattr(self, prop.private_name) | ||
|
|
||
| for mat in self._clear_on_rho_update: | ||
| if hasattr(self, mat): | ||
| delattr(self, mat) |
There was a problem hiding this comment.
Are we planning to keep these properties implemented in this way? Or should HasModel add these to the class based on the PhysicalProperty?
There was a problem hiding this comment.
Right now, it was mostly just a mechanism to invalidate the cache for those properties. I think it would best to update the caching mechanism on the HasModel class, but likely better for a separate PR.
There was a problem hiding this comment.
Handling the shape can likely be done better by defining where properties exist ("mesh.cell_centers", "mesh.faces", etc.) once we add that capability..
The metaclass now translates the deprecated xMap arguments (and raises if both x and xMap are given), but a subclass that forwards model= or xMap= up the super() chain used to hit object.__init__. HasModel.__init__ accepts both and the metaclass applies them after construction, so subclasses can keep setting their properties in any order. The model is only set once the constructor has finished, so reading self.model inside an __init__ gives None. The warnings about the constructor arguments and the xMap setter are now attributed to the caller by skipping only the frames in props.py, which also lets the pytest filters see simpeg using its own deprecated interface.
With rho parametrized, sim.sigmaMap = None used to clear rho's mapping too. Main leaves it alone, so do the same.
parametrize(sigma=A, rho=B) in one call raised nothing and silently kept only rho, it is a TypeError now. The class definition check no longer fires for a mixin that defines just one half of a pair, and it catches a reciprocal that does not point back. Giving neither of a pair also warns once now, instead of twice.
Keeps HasModel.model.fset(self, value) returning whether the model changed, as it did on main.
The 1D EM simulations passed h=None to a required property and warned on every default construction (h now defaults to None). Simulation3DDifferential defaulted mu before resolving the mu/mui pair, so mui= raised. The self potential simulation raises the ValueErrors from main again when neither or both of sigma and rho are given.
Sort the parametrize lists that came from sets (so collection order does not change between xdist workers), check flattened diagonal values on 2D and 3D meshes, and check the message of the removed PhysicalProperty arguments. Also add a check that every constructor that lost an xMap argument is keyword only, and that property defaults match the constructor defaults.
…recated map When a subclass adds sigmaMap= in its own super().__init__ call, the map is stashed and applied after construction. The intermediate __init__ still ran _init_recip_properties(sigma=None, rho=None), which warned about both being None, and a sigma value given next to the map was silently replaced. Both now look at the stashed parametrizations, so only the sigmaMap deprecation warns and a value plus a map raises the same TypeError as the top level path. Also note in the release notes that a subclass naming an xMap argument in its signature receives None for it.
Name the class that actually holds the shadowing attribute in the split reciprocal error, use a class with six properties to check the choices in the no parametrizations error are sorted (with three it passed on unsorted code about one run in six), and drop the xMap = None note from the breaking changes (it behaves as before).
|
Pushed another round of fixes. The one that mattered most is that Also in there: flattened anisotropic values, IP/SIP construction warnings, I put the deprecation at 0.27.0 and the removal at 0.29.0, with a draft I think I'll be happy with it in this state for merging to main and trying it. If anything it will just be even further edge cases, but I think I was able to catch most of them... |
PhysicalProperty takes components (an integer, or "dim" for the dimension of the mesh) for a property that has a vector at every location, like the remanent polarization in the magnetics differential simulation. The values and mapping outputs are checked against the mesh, and a value with a shape of (n, components) is stored flat in the order discretize uses. rem now has to have a length of 3 * n_cells (it accepted any length before, and rejected (n, 3) arrays).
The warning told people to call _prop_deriv, which is private. It now points at parametrizations.x.deriv(model) (and the docstring and release notes mention composing with ReciprocalMap when only the reciprocal is parametrized). The pytest filters for the warning match the new message.
Summary
Implements a parametrized interface to
PhysicalPropertiesPR Checklist
expect style.
to a Pull Request
@simpeg/simpeg-developerswhen ready for review.Reference issue
What does this implement/fix?
Additional information
Continuing on from #1593