@RossKnapman Thanks for your contribution. Very useful feature indeed!

Nevertheless, we mostly prefer not to break/change mumax3 default behaviour as it is stable for a couple of years now.

Therefore, If @JeroenMulkers and @JLeliaert don't mind, I would suggest you to rework your pull request a tiny bit. In particular, introduce API call RedefRegion(...) that internally calls your clear(...) function followed by DefRegion(...).

Also a few minor suggestions:

• please remove comments you added to DefRegion(...) function body, as those are not relevant to this pull request. Documenting mumax3 code through separate pull requests is very welcome though!
• please add some unit tests that validate your feature and check that the default behaviour.

I agree with @godsic . We should not break backwards compatibility at this point.

Furthermore, I would argue that what you describe as a problem is in fact intended behaviour. DefRegion(id, shape) updates the region id of cells inside a shape without modifying the region ids of cells outside the shape. The clear functionality you have implemented adds an additional side effect in DefRegion, which in some cases might be useful, but might be unwanted in other cases.

I do have another small comment on the clear function. In this function you 'clear' certain region ids by setting them to 0. This makes region 0 to be fundamentally different from the other regions, which was not the case before this commit.

I do believe that having a RedefRegion function could be useful. But in contrast to the clear function, it should not change the region ids to 0, but to a value specified by the user, e.g. RedefRegion(6, 43) updates the cells with region id 6, to have a region id 43.

@JeroenMulkers Region 0 is the default one. So it is not unreasonable to use it in RedefRegion(...), but more explicit version as you suggested is also fine with me.

Thank you for your comments, and apologies for the delayed response; things have been getting particularly busy with my PhD work over the past week.

I agree with the comment from @JeroenMulkers, that region 0 should not be made fundamentally different from the other regions. I will therefore implement the version of the function in which the user specifies the updated region ID.

Okay, I have re-worked the code as suggested, and added some unit tests. I hope that the code is acceptable in its current form. However, I see two potential issues that I would like to ask for your opinion on.

Firstly, the new function redefine, which is responsible for redefining the cells, works by looping through every cell in the system, and changing its region index to the new desired index if the index matches the old index. This seems kind of expensive. I am operating on the assumption that there is no kind of "central" region object which one can obtain all cells belonging to it from, but rather that the regions are solely stored cell-by-cell. If this is not the case, this could, of course, be improved, but examining the existing code leads me to believe that my implementation is the only viable method.

My second problem is that the hist field of the regions object is not updated when RedefRegion is called, like it is when DefRegion is called, as hist is a collection of functions that represent shapes, if I understand correctly. DefRegion, however, does not work in terms of shapes. I am not sure how important this is, and if it is really a problem, from the end user's perspective.

Hi,
I am working through the stale pull requests in this repo and have now arrived at this one.

The first problem you remarked, i.e. that the code loops through all cells, does not seem like a major issue to me. Other functions (e.g. regions.render, regions.Loadfile, regions.shift...) do the same. Those don't take prohibitively long, so I doubt doing the same here would be detrimental either.

The second problem seemed more severe to me, because not updating regions.hist would result in incorrect regions when resizing the simulation with setCellSize or setGridSize. I verified that this was indeed an issue by adding a resize operation at the end of your test script, see the figure below. To solve this, I adjusted RedefRegion so it now appends a suitable (though more expensive than usual) function to hist which preserves continuity in the history.

	Before resize	After resize
regions.hist problem
regions.hist fixed