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

Slope limiter testing #76

Merged
merged 6 commits into from
Sep 23, 2016
Merged

Slope limiter testing #76

merged 6 commits into from
Sep 23, 2016

Conversation

rbooth200
Copy link
Collaborator

@rbooth200 rbooth200 commented Sep 16, 2016

I've modified the slope limiters and written some scripts to produce some comparisons between the various slope limiters. Since this branch is built upon the slope_limiter branch we had before I will delete that pull request and we should use this one instead. This pull request includes the following:

  • updating the thermal properties after changes to the conserved quantities
  • recomputing the density for particles that become active, only if the density is out of date
  • I moved the various particle flags (active, potmin) to be part of the flags class.
  • Changes / generalizations to the slope limiter classes
  • Tests for the slope limiters.

I've also attached a couple of results from the slope limiter tests for the discussion. These can be used to rank the limiters by how diffusive they are. I'd suggest the order from least to most diffusive is:

  • Springel 2009
  • Balsara 2004
  • GIZMO
  • TVD Scalar
  • 1st Order Goduonv,

which agrees well with theory.

adsod_rarefaction
adsod_shock
gresho

@giovanni-rosotti
Copy link
Contributor

Looks fine to me - I think we could merge, although I'm not really familiar with the meshless and I think David should have the last word. After we have closed this and the tests branch I'll add the slope limiter tests to the tests run after every commit, at the moment we run only SPH tests.

@rbooth200
Copy link
Collaborator Author

It might be worth only running a subset of them since they take a few
minutes for each of the gresho tests.

On Thu, 22 Sep 2016 23:23 giovanni-rosotti, notifications@github.com
wrote:

Looks fine to me - I think we could merge, although I'm not really
familiar with the meshless and I think David should have the last word.
After we have closed this and the tests branch I'll add the slope limiter
tests to the tests run after every commit, at the moment we run only SPH
tests.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#76 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AOqItCIm4eNvwBX9aO9kT_ev9O8TxjG4ks5qsv_RgaJpZM4J-3U8
.

@dhubber
Copy link
Contributor

dhubber commented Sep 23, 2016

Just looking through some of the code and running through a batch of tests and all looks good so far. Will run a few more and if they're fine, I'll go ahead and merge.

@dhubber dhubber merged commit e5e434b into master Sep 23, 2016
@giovanni-rosotti giovanni-rosotti deleted the slope_limiter_testing branch September 23, 2016 09:58
@dhubber
Copy link
Contributor

dhubber commented Sep 23, 2016

All fine so I merged the branch. I guess we can now delete both the slope_limiter_testing and slope_limiters branch?

@rbooth200
Copy link
Collaborator Author

Please leave the slope_limiter_testing branch for now. I have some extra
bits and pieces that I'm still playing around with. I'll either push
more commits or delete it later...

On 23/09/16 10:58, dhubber wrote:

All fine so I merged the branch. I guess we can now delete both the
slope_limiter_testing and slope_limiters branch?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#76 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AOqItO9op6HqjETFb5U6po30z-xWXDjpks5qs6LigaJpZM4J-3U8.

@giovanni-rosotti
Copy link
Contributor

Sorry guys, deleted the branch as soon as I saw the e-mail - feel free to restore it if you need to

@rbooth200
Copy link
Collaborator Author

No worries, I'll do that later if need be.

On 23/09/16 11:03, giovanni-rosotti wrote:

Sorry guys, deleted the branch as soon as I saw the e-mail - feel free
to restore it if you need to


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#76 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AOqItD2cRsNx5kfG1FyUqv02Xc5OE6g2ks5qs6PigaJpZM4J-3U8.

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