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

Adding active particle docs #85

Merged
merged 9 commits into from Jul 9, 2019
Merged

Adding active particle docs #85

merged 9 commits into from Jul 9, 2019

Conversation

fearmayo
Copy link
Contributor

I'v e started to add/update the AP framework sections.

AP type none of the implementations have been (robustly) tested.

* SmartStar
* Accreting Particle
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a comment related to the Enzo code file not the documentation. In the file 'ActiveParticle_AccretingParticle.h' and 'ActiveParticle_SmartStar.h' there is a template class 'DepositMass' defined. One of the argument for this class is 'GalaxyParticleID' in both which I guess is an error getting carried over due to using the modified 'ActiveParticle_GalaxyParticle.h' for these two cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. That should be a separate PR though. Can you raise an issue on it or open a PR?

@kohdaegene
Copy link
Contributor

i realized that there might be some minimal conflicts with PR#83 which was just merged, where i had created some filler headings for active particles. other than that, this looks good to go. this PR should take precedence

Copy link
Contributor

@bwoshea bwoshea left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this as-is. I think it could possibly be expanded a bit, but as we use the active particles more it'll become more clear what's needed! :-)

@bwoshea
Copy link
Contributor

bwoshea commented Jul 4, 2019

Looks good to me! We need a third approve here.

Copy link
Contributor

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

I've marked some typos and suggestions that turn URLs into hyperlinks. I think you can accept these right on this page instead of implementing them by hand.

doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/physics/star_particles.rst Outdated Show resolved Hide resolved
doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/physics/active_particles.rst Outdated Show resolved Hide resolved
doc/manual/source/physics/active_particles.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

I didn't have permissions to apply these myself, but I'm happy to do so.

@jwise77
Copy link
Contributor

jwise77 commented Jul 8, 2019

Hi @fearmayo, can you review and accept @brittonsmith's suggested changes? Britton can approve when he's ready, and then I'll merge. This is the last substantial PR before the release!

Accepting suggestions made by Britton

Co-Authored-By: Britton Smith <brittonsmith@gmail.com>
@fearmayo
Copy link
Contributor Author

fearmayo commented Jul 9, 2019

Hi @fearmayo, can you review and accept @brittonsmith's suggested changes? Britton can approve when he's ready, and then I'll merge. This is the last substantial PR before the release!

I think that should be done now. I accepted the changes and I can see the commit.

Copy link
Contributor

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

All good by me.

@jwise77 jwise77 merged commit a37978f into enzo-project:master Jul 9, 2019
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.

None yet

6 participants