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

Remove vec from fluxfunction and fix compilation #1029

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from

Conversation

ykempf
Copy link
Contributor

@ykempf ykempf commented Sep 25, 2024

This includes #1026 at the moment, apologies.

Should address #940.

Adds fluxfunction to the tools built in github CI even though we don't use it (yet). That binary is not uploaded, either.

Fixes compilation of fluxfunction and removes the vectorclass from there, and the couple files it uses from particles/. Otherwise in particles/ only converting Vec3d to Vec3Dd so it compiles.

Neither fluxfunction nor particle pusher are tested so far!

Copy link
Contributor

@ursg ursg left a comment

Choose a reason for hiding this comment

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

Well, hum. Functional thumbs up, because a working fluxfunction is obviously better than a broken one.

But instead of partially keeping with vectorclass and partially unrolling vector operations into handcrafted scalar operations, maybe the better way to go here would be to just switch to Eigen vectors, like in other parts of the code (like the ionosphere)?

Also, std::vector<double> as a vector type instead of std::array<double,3> really doesn't look right to me.

@@ -60,79 +58,157 @@ struct Field
}
}

Vec3d getCell(int x, int y, int z) {
std::vector<double> getCell(int x, int y, int z) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest that this be std::array<double, 3>. Because why would you use a variable-sized vector here?

@markusbattarbee
Copy link
Contributor

+1 for using Eigen vectors instead.

@ursg
Copy link
Contributor

ursg commented Oct 18, 2024

This Pull request against this PR introduces Eigen vectors instead.

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