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

Addition of an unstructured lookup class #3128

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

Conversation

pmbremner
Copy link
Contributor

Follow-up for #3126. I added a new class to read in unstructured data tables. The class is modeled off of the ASCII data reader. The purpose in mind was to read in multiple unstructured material properties files, but this new class is not restricted to this purpose. The data files should contain columns of coordinates, and columns of data. The coordinates are stored in a searchable kdtree object, and the data are stored in vectors. The user can query the n-nearest neighbor data points from a target point. When applied to material properties, this allows the ability to update compositional properties on-the-fly based on current pressure and temperature conditions.

@pmbremner pmbremner mentioned this pull request May 31, 2019
Copy link
Member

@MFraters MFraters left a comment

Choose a reason for hiding this comment

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

I only looked at the header file yet, will do the rest later. Looks well documented to me! Just some minor fixes.

* needs to contain the number of columns and lines in the file:
* '#POINTS: TOTALNUMBERDATALINES NUMBERCOORDCOLUMNS NUMBERDATACOLUMNS'.
* For example '# POINTS: 5 4 3' would be 5 lines of data, 4 columns of coordinates,
* and 3 columns of data. The comments can optionally be followed by a
Copy link
Member

Choose a reason for hiding this comment

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

could you align the stars here?

* single line, which does not start with '#', containing the names of
* the data columns.
* The order of the following data columns has to be
* 'coordinates data' with @p querydim coordinate columns and @p components
Copy link
Member

Choose a reason for hiding this comment

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

so x,y,z,c1,c2,...? And should guerydim be dim?

public:

/**
* The following variables are properties of the material files
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean with material files?


virtual void initialize (const MPI_Comm &comm,
std::string datadirectory,
std::vector<std::string> material_file_names);
Copy link
Member

Choose a reason for hiding this comment

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

same, what do you mean with material?



/**
* Returns the data (velocity, temperature, etc. - according
Copy link
Member

Choose a reason for hiding this comment

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

e.g. velocity, ....?

/**
* Returns the data (velocity, temperature, etc. - according
* to the used plugin) for the n closest points to the target point.
* The function returns a vector of pairs of < data_return , Euclidean distance >,
Copy link
Member

Choose a reason for hiding this comment

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

please align stars :)

@pmbremner
Copy link
Contributor Author

Addressed comments

@tjhei tjhei mentioned this pull request Jun 1, 2019
Copy link
Member

@tjhei tjhei 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 pretty good already!

* The order of the following data columns has to be
* 'coordinates data' with @p querydim coordinate columns and @p components
* data columns. @p querydim and @p components need to
* match NUMBERCOORDCOLUMNS NUMBERDATACOLUMNS, respectivily.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* match NUMBERCOORDCOLUMNS NUMBERDATACOLUMNS, respectivily.
* match NUMBERCOORDCOLUMNS and NUMBERDATACOLUMNS, respectively.



// Create an instance of a kdtree object as a member variable of the class
KDTree<querydim> coordinate_kdtree;
Copy link
Member

Choose a reason for hiding this comment

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

this should be private

Copy link
Member

Choose a reason for hiding this comment

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

dealii::RTree<std::size_t, boost::geometry::index::linear<16>, dealii::IndexableGetterFromIndices<std::vector>>



virtual void initialize (const MPI_Comm &comm,
std::string datadirectory,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::string datadirectory,
const std::string &datadirectory,


virtual void initialize (const MPI_Comm &comm,
std::string datadirectory,
std::vector<std::string> file_name_list);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::vector<std::string> file_name_list);
const std::vector<std::string> &file_name_list);

* and instead reading them from the input file allows for more
* flexible files.
*/
UnstructuredDataLookup(const unsigned int components);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
UnstructuredDataLookup(const unsigned int components);
explicit UnstructuredDataLookup(const unsigned int components);

* data file it is checked that the length of this list is consistent
* with this number of components. This constructor is mostly provided
* for backwards compatibility. Not prescribing the number of components
* and instead reading them from the input file allows for more
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this constructor? I think the same one in AsciiDataLookup is only there for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. Perhaps I can try with and without this constructor in the unit_test, and decide from there.

* must have three compnents:
* TOTAL_NUMBER_DATA_LINES NUMBER_COORD_COLUMNS NUMBER_DATA_COLUMNS.
*/
std::vector<int> point_dimensions;
Copy link
Member

Choose a reason for hiding this comment

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

this might not be necessary to store as a member but a local variable might be enough, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

* are from the target point.
*/
std::vector< std::pair<std::vector<double>, double> >
get_data (const Point<querydim> &target_point, const unsigned int n_points) const;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be great to have a second function that only returns the closest point to make the return value simpler.

get_data (const Point<querydim> &target_point, const unsigned int n_points) const;

/**
* Returns a vector that contains the names of all data columns in the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Returns a vector that contains the names of all data columns in the
* Return a vector that contains the names of all data columns in the

@tjhei
Copy link
Member

tjhei commented Jun 1, 2019

Can you add a unit test similar to the one you showed us a couple of days ago?

}

// Set the coordinate points into the kdtree object
coordinate_kdtree.set_points (coordinate_values);
Copy link
Member

@tjhei tjhei Jul 14, 2021

Choose a reason for hiding this comment

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

kdtree=pack_rtree_of_indices(const ContainerType &container);

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