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

Bugfix for dustfrac and dust-to-gas ratio in dustydisc setup #273

Merged
merged 11 commits into from
Mar 30, 2022

Conversation

markahutch
Copy link
Contributor

Type of PR:
Bug fix
Modification to existing code

Description:
Dust fraction and dust-to-gas ratio was being computed incorrectly for one-fluid dust
Added additional versatility to the dustydisc setup to make it easier to define the initial power-law size distribution using any of the following combinations:
0) Ndust, smin, and smax

  1. Ndust, s1, and sN
  2. Ndust, s1, and logds
  3. Ndust, sN, and logds
  4. s1, sN, and logds
    Also laid the groundwork to make it possible to link the power-law size distribution from small grains and large grains during a hybrid setup.

Testing:
Compared the dust fractions and dust-to-gas ratios against an independent code
Tried different combinations of options to ensure the interactive setup wouldn't fail

Did you run the bots?
No

@danieljprice
Copy link
Owner

Hi @markahutch there is a build failure here that is not your fault due to a failure to download data files (raised in #275), however I think the MPI test failures need to be fixed before merge.

@markahutch
Copy link
Contributor Author

Hi @danieljprice,

I did notice that there were 6 failures on my fork after I pushed my commits to the repository, but none of them were MPI related. Instead, 5 were related to the build failures due to the inability to download files (as you mentioned) and 1 was a test failure that I could not reproduce on my system (the only log I could find on this issue said "GitHub Actions has encountered an internal error when running your job").

This is my first time trying to use the fork system with pull requests on Github so maybe I don't understand the process, but I am confused as to why the MPI tests passed on my fork, but not in the PR. I have not altered the code in the PR (at least knowingly) and I cannot reproduce the failures on my system. If I am looking at the log files correctly, the problem seems to be related to the following:

../src/tests/test_rwdump.F90:338:31:

338 | call allocate_memory(maxp_old)
| 1
Error: Type mismatch in argument ‘ntot’ at (1); passed INTEGER(4) to INTEGER(8)

However, I don't think that any of the changes I made are related to this issue and I can't get this to fail with or without MPI. Perhaps this is from the changes I pulled from upstream? I seem to recall the upstream changes had a green check next to them so this seems unlikely too. I have spent a few hours trying to figure out what I am missing, but to no avail. Do you have any ideas as to what I am doing wrong?

@danieljprice
Copy link
Owner

danieljprice commented Mar 18, 2022

I fixed the problem with the data file downloads, so I suggest to merge master -> your branch so at least this problem is fixed.

Currently some of the test suite with MPI seems to be failing, so to reproduce this you would need to compile with (e.g.):

make test MPI=yes

everything is passing on master so this is definitely some change coming from your branch. Would suggest to diff your branch against master to see where the offending line might be. I have also set the actions to rerun to make sure it is repeatable.

@danieljprice
Copy link
Owner

Also in general what would be great here is to add a unit test for the set_dustfrac routine so that this error cannot reoccur (e.g. into the src/tests/test_dust.f90 module)

@markahutch
Copy link
Contributor Author

I fixed the problem with the data file downloads, so I suggest to merge master -> your branch so at least this problem is fixed.

Currently some of the test suite with MPI seems to be failing, so to reproduce this you would need to compile with (e.g.):

make test MPI=yes

I've pulled in your changes, but "make test MPI=yes" does not produce any failures for me.

everything is passing on master so this is definitely some change coming from your branch. Would suggest to diff your branch against master to see where the offending line might be. I have also set the actions to rerun to make sure it is repeatable.

The diff only shows my commits so that would suggest that it is one of my changes, but I cannot for the life of me get anything to fail on my end and I get all green check marks online.

Also in general what would be great here is to add a unit test for the set_dustfrac routine so that this error cannot reoccur (e.g. into the src/tests/test_dust.f90 module)

I have already added a check in the set_dustfrac routine for this purpose that produces a fatal error if the dust-to-gas ratio is inconsistent with the one from the .setup file. Do you believe we need something more than this?

@danieljprice danieljprice merged commit 8155f6a into danieljprice:master Mar 30, 2022
s-neilson pushed a commit to s-neilson/phantom that referenced this pull request Mar 18, 2023
Bugfix for dustfrac and dust-to-gas ratio in dustydisc setup
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.

2 participants