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

Add forestclaw IO option #547

Merged
merged 38 commits into from
May 10, 2017
Merged

Conversation

mandli
Copy link
Member

@mandli mandli commented Nov 6, 2016

Included a bit of a refactor for the ASCII output as well so that
forestclaw could reuse as much of that code as possible.

Included a bit of a refactor for the ASCII output as well so that
forestclaw could reuse as much of that code as possible.
@mandli
Copy link
Member Author

mandli commented Mar 18, 2017

I am having two issues getting this to work correctly so if anyone has some ideas they would be most welcome:

  1. For reasons I don't understand and am having difficulty debugging some of the tests are importing the new ForestClaw IO routines which lead to errors as the non-ForestClaw Patch class is missing two attributes. This happens in only some of the tests so I am guessing somehow there's an import problem.
  2. The test in clawpack.forestclaw.io.forestclaw is not being found sometimes.

I could make this easier and only add these to the ForestClaw repository itself but I was thinking it would be nice to start to build up the infrastructure for having a Python interface to ForestClaw.

@mandli
Copy link
Member Author

mandli commented Mar 18, 2017

Strangely tests are passing on travis beyond the issues with Python 3.x. Still not seeing the test for ForestClaw though.

@coveralls
Copy link

coveralls commented Mar 18, 2017

Coverage Status

Coverage decreased (-6.4%) to 39.481% when pulling 158d895 on mandli:add-forestclaw-reading into 986a3d1 on clawpack:master.

@coveralls
Copy link

coveralls commented Mar 18, 2017

Coverage Status

Coverage decreased (-6.4%) to 39.481% when pulling 725bcfd on mandli:add-forestclaw-reading into 986a3d1 on clawpack:master.

@ketch
Copy link
Member

ketch commented Mar 18, 2017

You probably already know this but the test failures on Travis seem like they would be fixed by using range instead of xrange. See http://stackoverflow.com/questions/17192158/nameerror-global-name-xrange-is-not-defined-in-python-3.

@ketch
Copy link
Member

ketch commented Mar 18, 2017

The test in clawpack.forestclaw.io.forestclaw is not being found sometimes.

I believe nose does not pick up functions with test in the name that are not in a file with test in the name. Running nosetests in forestclaw/io does nothing, but if I rename forestclaw.py to test.py then it runs the test.

def test_forestclaw_patch():
"""Test the simple extension of the pyclaw.Patch class"""

patch = Patch(pyclaw.geometry.dimension(0.0, 1.0, 10))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I run this test, I get an error because dimension is not capitalized. Changing it to Dimension (the class name) fixes that and the test passes.

@ketch
Copy link
Member

ketch commented Mar 18, 2017

The refactor of ascii.py is definitely an improvement. Thanks!

@mandli
Copy link
Member Author

mandli commented Mar 18, 2017

I see, I was confused to where the xrange would have come from given that io.ascii was working before but then realized it was my fault. Should be working again.

@mandli
Copy link
Member Author

mandli commented Mar 18, 2017

I realized that it would be better to call the io module in the forestclaw package ascii to align better with petclaw and that this is really just a slightly changed ascii format for ForestClaw.

@mandli
Copy link
Member Author

mandli commented Mar 18, 2017

I think I may have confused myself. If forestclaw has it's own geometry module then do I need to also import all of the PyClaw versions of the classes I am not overriding or is there a way to do that more simply? It looks like PetClaw does all the classes but tends to modify all of them as well so I am not exactly certain what the right thing to do here is.

Also include an actual test.
@mandli
Copy link
Member Author

mandli commented Mar 18, 2017

Note now with the move to have forestclaw be a package at the level of petclaw and pyclaw that the clawpack/clawpack repository will also need an update in the setup.py file so that the link to forestclaw will be taken care of.

@coveralls
Copy link

coveralls commented Mar 18, 2017

Coverage Status

Coverage decreased (-0.06%) to 45.783% when pulling c2a91c8 on mandli:add-forestclaw-reading into 986a3d1 on clawpack:master.

@ketch
Copy link
Member

ketch commented Mar 20, 2017

I think I may have confused myself. If forestclaw has it's own geometry module then do I need to also import all of the PyClaw versions of the classes I am not overriding or is there a way to do that more simply? It looks like PetClaw does all the classes but tends to modify all of them as well so I am not exactly certain what the right thing to do here is.

You need to import them only if you wish to be able to access them under the ForestClaw namespace. For instance, petclaw.geometry does not include a definition of a Grid class, so there is no clawpack.petclaw.Grid. Internally, PetClaw just uses the clawpack.pyclaw.Grid class. Perhaps it would make sense to do from pyclaw import Grid there, but nobody seems to care so far.

@mandli
Copy link
Member Author

mandli commented Mar 21, 2017

I kind of want to mimic what is possible with PetClaw as much as possible but I will try some things. I still actually do not have the tests passing on my own machine due to a weird import conflict but I think I have seen something weird like this before. Given that the Travis tests are working maybe this is ready to merge then.

@ketch
Copy link
Member

ketch commented Mar 21, 2017

Care to post the error you're getting locally?

@ketch
Copy link
Member

ketch commented Apr 16, 2017

Oops, I was wrong. To fix the current Travis error, you just need to change io -> fileio on line 11 of pyclaw/src/pyclaw/setup.py.

@mandli
Copy link
Member Author

mandli commented Apr 17, 2017

I just opened a PR for clawpack/clawpack#110. Still working out the testing issues.

@mandli
Copy link
Member Author

mandli commented Apr 19, 2017

Made some incremental changes and bug fixes. I am hoping that this can soon be merged in. @donnacalhoun and @ketch I may need to have you test these things out given that this will be a multi-repository commit.

@donnaaboise
Copy link
Contributor

Thanks for all of your work on this! I'll check it out in the next few days.

@ketch
Copy link
Member

ketch commented Apr 28, 2017

Never mind; the issue is that we're getting this when we install pyclaw:

Warning: Subpackage 'clawpack.pyclaw.fileio' configuration returned as 'clawpack.pyclaw.io'

This seems to come from

https://github.com/numpy/numpy/blob/master/numpy/distutils/misc_util.py#L925

but I have no idea why. There must still be a reference to pyclaw.io somewhere...?

I think the only important change here is the one in
pyclaw/src/pyclaw/fileio/setup.py.
@ketch
Copy link
Member

ketch commented Apr 28, 2017

Okay, my latest PR fixes it. I think that with that we are ready to merge.

@coveralls
Copy link

coveralls commented Apr 28, 2017

Coverage Status

Coverage decreased (-0.05%) to 45.78% when pulling e5f1e9a on mandli:add-forestclaw-reading into 31aa16d on clawpack:master.

@ketch
Copy link
Member

ketch commented Apr 28, 2017

Yeah, I think this doesn't pass yet because it needs the clawpack/clawpack PR.

@mandli
Copy link
Member Author

mandli commented Apr 28, 2017

Does this work for you then?

@mandli
Copy link
Member Author

mandli commented Apr 29, 2017

I am a bit concerned that there is still something not working quite right but maybe we can merge this in and see where non-travis testing might give us.

@coveralls
Copy link

coveralls commented May 7, 2017

Coverage Status

Coverage decreased (-0.05%) to 45.78% when pulling 33c066a on mandli:add-forestclaw-reading into 047f1c1 on clawpack:master.

@mandli mandli merged commit 3f06f91 into clawpack:master May 10, 2017
@ketch ketch removed the in progress label May 10, 2017
@donnaaboise
Copy link
Contributor

We are having trouble getting the read function in pyclaw/ascii to use the read_patch_header in forestclaw.fileio.ascii. So either we shouldn't' be in pyclaw.fileio.ascii.read at all, or somewhere, the correct header function is not getting assigned. We have set format='forestclaw' in the setplot file, but this doesn't seem to be enough. In looking at forestclaw.fileio.asii, the only reference to a read function is to the pyclaw version.

@mandli
Copy link
Member Author

mandli commented May 14, 2017

This might be best in an issue instead. In any case I am in the midst of trying to replicate the problem in the swirl ForestClaw example. Hopefully I can identify what's wrong.

@donnaaboise
Copy link
Contributor

I'd be happy to start an issue, it that helps.

@mandli
Copy link
Member Author

mandli commented May 14, 2017

I think that would be a bit cleaner given that this is already merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants