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

Fix SPH answer tests in yt-4.0 #2208

Merged
merged 4 commits into from
Jul 18, 2019
Merged

Conversation

jzuhone
Copy link
Contributor

@jzuhone jzuhone commented Mar 19, 2019

The sph_answer tests need updating for yt-4.0. This PR is a WIP to update them. Things that have to be done:

  • Remove deposit fields from testing in SPH frontends (except tipsy, which doesn't use sph_answer)
  • Use raw particle types to compute particle sizes
  • Fix projections for non-region objects

@jzuhone
Copy link
Contributor Author

jzuhone commented Mar 22, 2019

So these modifications to sph_answer are now working for every dataset which uses it except two: the FLASH particle dataset (which only tests deposit fields) and TipsyGalaxy, which is giving some strange errors like this:

RuntimeError: Error: yt attempted to read outside the boundaries of a non-periodic domain along dimension 1.
Region left edge = -2675.6919323730467 code_length, Region right edge = 2591.2012097167967 code_length
Dataset left edge = -557.3009106445312 code_length, Dataset right edge = 472.81018798828126 code_length

I suspect that this this has to do with the way the Tipsy datasets have to calculate their boundaries? I don't know much about this data at all, however.

@jzuhone jzuhone changed the title [WIP] Fix SPH answer tests in yt-4.0 Fix SPH answer tests in yt-4.0 Mar 22, 2019
@ngoldbaum
Copy link
Member

It's because that dataset doesn't have periodic boundary conditions and something in the test is causing a read outside the boundary. Should probably be looked into more.

@jzuhone
Copy link
Contributor Author

jzuhone commented Mar 22, 2019

@ngoldbaum for this PR or separate?

@ngoldbaum
Copy link
Member

It doesn't really matter. If you want we can merge this now and come back to it in the context of the PR merging with master.

@jzuhone
Copy link
Contributor Author

jzuhone commented Mar 22, 2019

Selfishly, I'd prefer the latter so we can also get the Arepo PR merged in.

@ngoldbaum
Copy link
Member

I don't think this needs to be merged before the arepo one.

@jzuhone
Copy link
Contributor Author

jzuhone commented Mar 22, 2019

Well, the only way to make sure the Arepo tests pass is to merge this one first. No hurry of course.

@ngoldbaum
Copy link
Member

That would be true if we were running the SPH answer tests, which we aren't right now. This one only needs to be merged before we get rid of the yt-4.0 branch.

@matthewturk
Copy link
Member

@jzuhone I'm not entirely sure that I understand how to fix the issue of growing the domain, which I think is what's happening here. Perhaps the simplest thing to do would be to -- for cases that are non-periodic and particle-based -- just say it's okay if we read outside the domain for particle fields.

@matthewturk
Copy link
Member

@jzuhone I think a check for the region to see if the Index type is a particle type and if the domain is non-periodic would be fine; just swallow the error in that case.

@jzuhone
Copy link
Contributor Author

jzuhone commented Jul 18, 2019

This is ready to go, we can handle the Tipsy issue separately.

@matthewturk matthewturk merged commit e4fc8cf into yt-project:yt-4.0 Jul 18, 2019
@jzuhone jzuhone deleted the fix_sph_answer branch October 10, 2019 13:45
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.

3 participants