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

horizon find and cartesian grid dump #616

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HengruiZhu99
Copy link
Collaborator

dumping data on Cartesian grid with application to horizon finder

@HengruiZhu99 HengruiZhu99 marked this pull request as ready for review November 18, 2024 15:10
Copy link
Collaborator

@dradice dradice left a comment

Choose a reason for hiding this comment

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

I am not sure why we would want to convert the tracker list to a list of pointers, which is what is being done here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better not to make pos public and instead use GetPos() to get the compact object position

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be better not to make pos public and instead use GetPos() to get the compact object position

Done!

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • I do not understand the logic for why parfilename is given as a parameter to the function and then overwritten. Also, there is no guarantee that parfilename will really be 100 characters wide, so it would be safer to pass a char const * if you want to have the filename as input.
  • You should not use sprintf, use snprintf instead (or use C++ strings)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed! Originally I wanted to run the ahfinder as soon as the file is dumped, so I'm saving the name of the parfile. But I then decided against that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you using make_unique instead of emplace_back? I think it is better to have a list of objects, rather than a list of pointers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using the smart pointer to avoid complications of memory management using std::vector

@HengruiZhu99
Copy link
Collaborator Author

I am not sure why we would want to convert the tracker list to a list of pointers, which is what is being done here.

I mainly want to use the std::vector because of the easiness of accessing the elements. For the std::list I need an iterator making it more lengthy to pass the position of puncture into the horizon dump object.

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