Skip to content

Mesh for anisotropic particles#52

Merged
mphoward merged 6 commits intorefactor-anisofrom
feature/mesh-aniso
Jun 12, 2025
Merged

Mesh for anisotropic particles#52
mphoward merged 6 commits intorefactor-anisofrom
feature/mesh-aniso

Conversation

@mayukh33
Copy link
Contributor

@mayukh33 mayukh33 commented Apr 4, 2025

No description provided.

BoundaryType upper_bc);

std::shared_ptr<Mesh> slice(int start, int end) const;
std::shared_ptr<Mesh> slice(std::vector<int> start, std::vector<int> end) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to pass all these vectors by const reference to avoid copies

Suggested change
std::shared_ptr<Mesh> slice(std::vector<int> start, std::vector<int> end) const;
std::shared_ptr<Mesh> slice(const std::vector<int>& start, const std::vector<int>& end) const;


//! Get position on the mesh, defined as center of bin
double center(int i) const;
std::vector<double> center(std::vector<int> i) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::vector<double> center(std::vector<int> i) const;
std::vector<double> center(const std::vector<int>& i) const;


//! Get lower bound of bin
double lower_bound(int i) const;
std::vector<double> lower_bound(std::vector<int> i) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::vector<double> lower_bound(std::vector<int> i) const;
std::vector<double> lower_bound(const std::vector<int>& i) const;


//! Get upper bound of bin
double upper_bound(int i) const;
std::vector<double> upper_bound(std::vector<int> i) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::vector<double> upper_bound(std::vector<int> i) const;
std::vector<double> upper_bound(const std::vector<int>& i) const;

BoundaryType upper_boundary_condition() const;

int asShape(double dx) const;
std::vector<int> asShape(std::vector<double> dx) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::vector<int> asShape(std::vector<double> dx) const;
std::vector<int> asShape(const std::vector<double>& dx) const;

Comment on lines +70 to +72
double integrateSurface(std::vector<int> idx, double j_lo, double j_hi) const;
double integrateSurface(std::vector<int> idx, const DataView<double>& j) const;
double integrateSurface(std::vector<int> idx, const DataView<const double>& j) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful here. Isn't j now going to need to be a vector? Or 1 vector per face of the mesh site?

Comment on lines +74 to +76
double integrateVolume(std::vector<int> idx, double f) const;
double integrateVolume(std::vector<int> idx, const DataView<double>& f) const;
double integrateVolume(std::vector<int> idx, const DataView<const double>& f) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
double integrateVolume(std::vector<int> idx, double f) const;
double integrateVolume(std::vector<int> idx, const DataView<double>& f) const;
double integrateVolume(std::vector<int> idx, const DataView<const double>& f) const;
double integrateVolume(const std::vector<int>& idx, double f) const;
double integrateVolume(const std::vector<int>& idx, const DataView<double>& f) const;
double integrateVolume(const std::vector<int>& idx, const DataView<const double>& f) const;

Comment on lines +81 to +83
virtual double gradient(std::vector<int> idx, double f_lo, double f_hi) const = 0;
double gradient(std::vector<int> idx, const DataView<const double>& f) const;
double gradient(std::vector<int> idx, const DataView<double>& f) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here, won't the gradient now return a vector?


template<typename T>
typename std::remove_const<T>::type Mesh::interpolate(double x, const DataView<T>& f) const
typename std::remove_const<T>::type Mesh::interpolate(std::vector<double> x,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this implementation right? Don't we need to do multidimensional interpolation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on our discussion, this interpolation method probably needs to be updated so that it is operating only on one coordinate, with the rest fixed to a specific bin.

src/mesh.cc Outdated
: lower_(lower_bound), shape_(shape), lower_bc_(lower_bc), upper_bc_(upper_bc), start_(0)
{
step_ = (upper_bound - lower_bound) / shape_;
std::vector<double> step_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Step is already a member variable

Suggested change
std::vector<double> step_;

@mayukh33 mayukh33 requested a review from mphoward April 18, 2025 03:44
* \param i Multidimensional bin index
* \return Upper bound of the multidimensional bin
*/
virtual double area(int i) const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

For this method, what area is this going to return? Should it return the areas of all lower faces in some defined order? Or, should it take both an index to a bin and an index to a dimension and return the area of that lower face?

Or perhaps, it needs three arguments: bin, face, and +/- direction?

* \param x Coordinate position of the bin
* \return Bin index
*/
std::vector<int> bin(double x) const;
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 going to need to take a vector, right?

* \param i Multidimensional bin index
* \return Volume of the bin
*/
virtual double volume(int i) const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a vector


template<typename T>
typename std::remove_const<T>::type Mesh::interpolate(double x, const DataView<T>& f) const
typename std::remove_const<T>::type Mesh::interpolate(std::vector<double> x,
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on our discussion, this interpolation method probably needs to be updated so that it is operating only on one coordinate, with the rest fixed to a specific bin.

Comment on lines +137 to +144
//! Get the integral of the surface over the mesh
/*!
* \param shape Shape of the mesh
* \return Length of the mesh
*/
double integrateSurface(const std::vector<int>& idx, double j_lo, double j_hi) const;
double integrateSurface(const std::vector<int>& idx, const DataView<double>& j) const;
double integrateSurface(const std::vector<int>& idx, const DataView<const double>& j) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you think about how these are going to work in the new code? Is it going to integrate over a specific face, or all of them? And what will it integrate?

@mphoward
Copy link
Contributor

mphoward commented Jun 5, 2025

@mayukh33 can this be merged into refactor-aniso before I work on it? It's OK if it isn't perfect so long as it isn't totally broken, since I will likely be making quite a few changes anyway.

@mayukh33
Copy link
Contributor Author

mayukh33 commented Jun 5, 2025

Hi! I think it can be merged into refactor-aniso. I also wanted to make a note that I had to change the black-pre-commit-mirror.yaml's black-pre-commit-mirror requirement from 24.10.0 to 24.8.0. Otherwise, it gave an error that it could not find the 24.10.0 among the list of supported distributions.

@mphoward
Copy link
Contributor

mphoward commented Jun 5, 2025

Please revert that change, and I'll have a look at it locally

@mayukh33 mayukh33 requested a review from mphoward June 11, 2025 14:24
@mphoward mphoward merged commit 009f2d3 into refactor-aniso Jun 12, 2025
1 check passed
@mphoward mphoward deleted the feature/mesh-aniso branch June 12, 2025 14:45
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