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

Add scalar template parameter in magnetic field #247

Closed

Conversation

beomki-yeo
Copy link
Collaborator

@beomki-yeo beomki-yeo commented Apr 24, 2022

For #246

@beomki-yeo beomki-yeo requested a review from krasznaa April 24, 2022 01:10
class constant_magnetic_field {

public:
using vector3 = __plugin::vector3<scalar>;
using point3 = __plugin::point3<scalar>;
using vector3 = __plugin::vector3<scalar_t>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might be what @krasznaa you mentioned during the chat?
plugin-specific type should also be templated if I am correct

Copy link
Member

Choose a reason for hiding this comment

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

Yes. In this class's case I would imagine making it templated on the vector3 and point3 types. Like:

template <typename vector3_t, typename point3_t, typename context_t = dindex>
class constant_magnetic_field {

Since the scalar type is only used for defining these two types in this class, we can even forego declaring those types as templates. So, we would specify a concrete type like:

using b_field_t = constant_magnetic_field<algebra::eigen::vector3<float>, algebra::eigen::point3<float> >;

I didn't try any of this out, but I think something like that could work.

@beomki-yeo beomki-yeo marked this pull request as draft April 25, 2022 18:18
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

This looks good! Let's not try to bite off more than we can chew. We can come back to generalising the templates even further in future PRs.

class constant_magnetic_field {

public:
using vector3 = __plugin::vector3<scalar>;
using point3 = __plugin::point3<scalar>;
using vector3 = __plugin::vector3<scalar_t>;
Copy link
Member

Choose a reason for hiding this comment

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

Yes. In this class's case I would imagine making it templated on the vector3 and point3 types. Like:

template <typename vector3_t, typename point3_t, typename context_t = dindex>
class constant_magnetic_field {

Since the scalar type is only used for defining these two types in this class, we can even forego declaring those types as templates. So, we would specify a concrete type like:

using b_field_t = constant_magnetic_field<algebra::eigen::vector3<float>, algebra::eigen::point3<float> >;

I didn't try any of this out, but I think something like that could work.

@krasznaa
Copy link
Member

You'll need to resolve the conflict before it could be merged though. 😉

@niermann999
Copy link
Contributor

Is this resolved with #291?

@beomki-yeo
Copy link
Collaborator Author

beomki-yeo commented Sep 8, 2022

Um no. I think magnetic field should still take scalar_type rather than transform_type.

@stephenswat
Copy link
Member

I think this can be closed now that #311 is merged, right?

@beomki-yeo
Copy link
Collaborator Author

yes

@beomki-yeo beomki-yeo closed this Oct 13, 2022
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.

4 participants