Skip to content

spherical sampling#5

Open
tjhei wants to merge 10 commits into
integrated-earth:mainfrom
quangx:spherical
Open

spherical sampling#5
tjhei wants to merge 10 commits into
integrated-earth:mainfrom
quangx:spherical

Conversation

@tjhei

@tjhei tjhei commented Jul 27, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@tjhei

tjhei commented Jul 27, 2024

Copy link
Copy Markdown
Contributor Author
  1. can you please rebase to the most current main branch?
  2. can you run make indent in the top folder?

@tjhei tjhei left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thank you.

Comment thread sample/sample.cc
StructuredData structured_data(min,max,num_pts,num_components,false);
std::vector<bool> processed(num_components);

std::cout<<num_components;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this should probably be removed?

Comment thread sample/sample.cc
return structured_data;

}
StructuredData write_to_vertex_spherical(const std::array<unsigned int,3> &num_pts)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

why is this called "write to vertex"?

Comment thread sample/sample.cc
Comment on lines +179 to +184
std::vector<bool> processed(num_components);


for(unsigned int i=0;i<num_components;++i){
processed[i]=false;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
std::vector<bool> processed(num_components);
for(unsigned int i=0;i<num_components;++i){
processed[i]=false;
}
std::vector<bool> processed(num_components, false);

Comment thread sample/sample.cc
}
}
else{
std::cout<<"unacceptable number of arguments";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

can you print a help text how to use this code?

Comment thread sample/structured.h


}
StructuredData(){

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is this constructor necessary?

Comment thread sample/structured.h
StructuredData(){

}
Point<3,double> spherical_to_cartesian_coordinates(const std::array<double,3> &spherical_coord){

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
Point<3,double> spherical_to_cartesian_coordinates(const std::array<double,3> &spherical_coord){
Point<3,double> spherical_to_cartesian_coordinates(const std::array<double,3> &spherical_coord) const {

Comment thread sample/structured.h
return cartesian_coord;

}
Point<3,double> spherical_to_cartesian_coordinates(Point<3,double> &spherical_coord){

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
Point<3,double> spherical_to_cartesian_coordinates(Point<3,double> &spherical_coord){
Point<3,double> spherical_to_cartesian_coordinates(Point<3,double> &spherical_coord) const{

Comment thread sample/structured.h
Comment on lines +86 to +89

}
std::array<double,3>
cartesian_to_spherical_coordinates(const Point<3> &position)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in general, can you remove unnecessary empty lines within functions but add them between functions?

Comment thread sample/structured.h
}
return location;
}
Point<3,double> index_to_location_spherical(const //returns cartesian coordinates given an index

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is an uncommon way to comment a function!

@tjhei tjhei left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please delete the two files and squash your commits.

You will need to rebase to the current master to be able to merge.

I know it is incomplete, but maybe we should merge and improve from there? Do you have other incomplete work that should be put in or can we merge?

Comment thread step-40/compile_commands.json Outdated
@@ -0,0 +1,7 @@
[

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

please remove this file

Comment thread step-40/cmake_install.cmake Outdated
@@ -0,0 +1,54 @@
# Install script for directory: /home/quang/Documents/viz-tools/step-40

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

please remove this file

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