Raise Warning if the Capacity Array is Not Set But mapc2p is

[NDM14]

Changes:

• added method is_mapped_state_valid to state class to check whether the capacity array and its index is set when mapc2p is also set.
• added method is_mapped_solution_valid to check whether every state has a valid mapping at runtime.
• added test class and test to check whether the updated is_valid method works as intended

The names of the methods are debatable. I tried to I adapt to the existing structure.
Also the ValueError can be exchanged to Warnings or bools. Depending on want you all want.
Raising an ValueError seemed more logical, because the program should break, when the mapping isn't valid, but can go through when no mapping isn't present. That's why I used the boolean there.
To give out logging with a boolean value seems a little bit outdated. I can also introduce errorhandling somewhere, but I'm not sure where it should be. Maybe that should be done when a simulation is created.

[ketch]

I'd like to see this implemented following the pattern we use everywhere else:

• each object has a single "is_valid" function
• All validity tests are done inside that function

This ensures that higher level code doesn't need to know details about the object it is using.

If mapc2p is set, you should check that the capacity index is set to an integer between zero and num_aux - 1, inclusive.

[NDM14]

I hope this is better now. I added the checks to the is_valid method from the state object and removed the new method from the solution object. I also added instructions the the errors to guide the user to the right settings.

[ketch]

This looks good, modulo one comment I left above. Have you tested this? It would be great to include a test in the PR.

[NDM14]

I would happly add a unittests. The questions is where should I add it to? I can only see regression tests in you test folder

[NDM14]

@ketch I added a unit test to the test folder. I added it into a folder called unittest to not interfere with the current test structure. I took the setup from the example mentioned in the issue #614 to get a valid setup to run the tests. I included every possible error in the tests to check whether every error is reached at the right time. I know that the project currently just tests via regression tests or tests the example separately, but I would propose to include this test and maybe expand unit testing to be sure about new code coming into the project. It also could be included in GitHub Actions or pre-hooks.

[NDM14]

@ketch @mandli Hey guys, I wanted to ask what do you think about the test? It would be really nice if we could merge this next week. If you don't like the test, can you maybe tell me what you don't like about it / expect from the test or where I should put another test? I'm here to learn and help :)

Best regards!

[mandli]

Something failed that isn't your test, but I assume is related to the PR. Need to run it locally to see what's going on. May not be able to get to this until Monday though.

Regarding the test itself, I am not sure that another subdirectory may be necessary here given that the existing test in "tests" is also a unit test.

[NDM14]

@mandli Hey thanks for the reply. I ran it locally and the "problem" is with the checks. The valid function works "as intended" and checks for the capacity array to be set or for example the index to be set. Now there are the examples where this is not the case, so the check fails and the error is thrown. With the mapped examples everything works fine. This was the reason I proposed a new method to check mapped cases separately. What do you propose? Can we maybe somehow add a check to the is_valid function, so the checks will only be invoked when the mapping is used?

[ketch]

It looks like you're checking whether mapc2p is None. However, by default it seems that the code sets mapc2p to a value that is not None, so your error will get tripped every time no mapping is used. Obviously, that's not what we want. If you look at line 263 of pyclaw/geometry.py, you can see how we currently check if a mapping is set:

        if self.mapc2p in list(identity_map.values()):

I think you should use the same check.

[NDM14]

@ketch Is there a way, that the examples are set up wrong? When no, what am I missing? When I run the code with the check you suggested, the mapped examples work, but the examples with no mapping don't. It doesn't seem that the check for the mapping is the problem. It should be easy, but I think I just can't get it right for some reason. I tried multiple things, but nothing seems to work the way its intended :(

[NDM14]

[ketch]

I realized what the problem was: we create a dict of identity maps in geometry.py and in state.py. The maps are the same, but of course the functions in them have different memory addresses so to Python they are not the same. I've just pushed a fix -- instead of creating this map again in state.py, I just import it from geometry. That way, they are exactly the same dict and the same maps, so the check you are doing will work.

[coveralls]

coverage: 47.046% (+0.4%) from 46.602%
when pulling 2f526ee on NDM14:feature/614/raise-warning-capacity
into 8854071 on clawpack:master.

[NDM14]

Oh my god, thank you a lot! I was thinking about that, but was just lazy and wanted to test it quickly and then just change it wenn it would work. That's a lessons learned😅🤦‍♂️

[ketch]

@mandli if you agree, I think this is ready to merge.

@NDM14 thanks for your contribution! Nice work, and welcome to the Clawpack development community!

[NDM14]

Your welcome and thank you too! Happy to help :)