Add inline documentation for the DagMC and dagmcmetadata classes. #945

pshriwise · 2024-02-26T05:45:07Z

Please follow these guidelines when making a Pull Request.
This template was adapted from here and here.

Description

Adding some inline documentation so we can start leveraging it for rendered documentation.

gonuke

Looks like you ended up doubling a line here causing a build error.

src/dagmc/dagmcmetadata.hpp

ahnaf-tahmid-chowdhury

Thank you, @pshriwise, for creating this documentation support. It has been very helpful in understanding the source code. I have a few formatting suggestions, although they are not absolutely necessary.

src/dagmc/dagmcmetadata.hpp

ahnaf-tahmid-chowdhury · 2024-02-26T16:44:26Z

src/dagmc/dagmcmetadata.hpp

+   * @brief Constructs a new DagmcMetadata object.
+   *
+   * @param dagmc A pointer to the DagMC instance associated with this metadata.
+   */  // Constructor


Suggested change

*/ // Constructor

*/

gonuke

Thanks for this great effort @pshriwise. I have a few necessary changes (typos, etc) and a few additional requests, although I feel a little sheepish "punishing" your effort with additional requests.

gonuke · 2024-02-26T16:35:58Z

src/dagmc/DagMC.hpp

-
+  /**
+   * @brief Determines whether a given point is inside a specified volume. This
+   * function is slower than point_in_volume.


Is it fair to say that this method is more robust? That is, should we provide some indication about when/why to use a slower method?

I have a sense that it is more robust, but am not terribly familiar (or am no longer terribly familiar) with it myself. Do you recall in what situations we should use it over the point_in_volume method? I'd be happy to add them if so.

For now, I'll include where the method originates so a developer can make a decision based on the publication.

gonuke · 2024-02-26T16:36:42Z

src/dagmc/DagMC.hpp

+   * @param xyz A 3-element array representing the coordinates of the point.
+   * @param[out] volume The volume containing the point.
+   * @param uvw Optional. A 3-element array representing the unit direction to
+   * be used when firing a test ray. Default is NULL.


Will this also use a random direction if nothing is specified?

Yep yep. I'll include that.

gonuke · 2024-02-26T16:38:20Z

src/dagmc/DagMC.hpp

+   * @param[out] result The result of the operation. If present and
+   * non-empty, the history is used to look up the surface facet at which the


It looks like there is an entry missing and that this description applies to the RayHistory and not this results parameter

Mea culpa. Thanks!!

gonuke · 2024-02-26T16:51:34Z

src/dagmc/DagMC.hpp

+   * @brief Gets the angle between a specified surface and a ray from a given
+   * point in a specified direction.


Not sure why we named this get_angle but it appears to return the surface normal of the facet, and not an angle...

I'm not sure either, but probably not going to tackle that here. I did try to add some clarity to the brief description though.

gonuke · 2024-02-26T16:52:53Z

src/dagmc/dagmcmetadata.hpp

+  /**
+   * @brief Constructs a new DagmcMetadata object.
+   *
+   * @param dagmc A pointer to the DagMC instance associated with this metadata.


Suggested change

* @param dagmc A pointer to the DagMC instance associated with this metadata.

* @param DAGptr A pointer to the DagMC instance associated with this metadata.

gonuke · 2024-02-26T16:53:16Z

src/dagmc/dagmcmetadata.hpp

+   * @brief Constructs a new DagmcMetadata object.
+   *
+   * @param dagmc A pointer to the DagMC instance associated with this metadata.
+   */  // Constructor


Shouldn't we include the two optional parameters?

gonuke · 2024-02-26T17:00:01Z

src/dagmc/dagmcmetadata.hpp

  bool try_to_make_int(std::string value);

  // private member functions
 private:
-  // parse the material data
+  /**
+   * @brief Parses the material data from the associated DagMC instance.


What does this actually do? Changes the state of some data structures in some way, but how? (And similarly for following methods)

gonuke · 2024-02-26T17:28:56Z

src/dagmc/dagmcmetadata.hpp

  std::map<moab::EntityHandle, std::vector<std::string>>
  get_property_assignments(std::string property, int dimension,
                           std::string delimiters,
                           bool remove_duplicates = true);

-  // remove duplicate properties from the vector of properties
+  /**
+   * @brief Removes duplicate properties from a vector of properties.


Is it possible to give a simple example of how this might manifest itself? Like the neutron example you gave below for the set_remove_rich() method?

gonuke · 2024-02-26T17:30:33Z

src/dagmc/dagmcmetadata.hpp

@@ -8,118 +8,316 @@

 class dagmcMetaData {


This class would probably also benefit from some top level general description of how it all works, e.g. what's a "property" and how are key:value pairs specified, etc...

Gave it a go. Lemme know what you think.

Co-authored-by: Ahnaf Tahmid Chowdhury <[email protected]>

pshriwise · 2024-02-27T03:55:43Z

src/dagmc/dagmcmetadata.hpp

+
+  /**
+   * @brief Finalizes the counters after all data has been parsed.
+   */
  void finalise_counters();


I didn't see a definition for this method and have removed it.

gonuke

One small clarification

gonuke · 2024-02-27T11:35:28Z

src/dagmc/DagMC.hpp

  ErrorCode surface_sense(EntityHandle volume, EntityHandle surface,
                          int& sense_out);

+  /**
+   * @brief Gets the angle between the normal of a specified surface and a ray


I don't think this gets an angle. I think it just gets the facet normal at the location of the surface crossing. (Hence my incomplete comment on the poor choice of name... that I don't expect you to fix.)

Ah, yeah you're right. Sorry. In a rush updating this last night. On it.

ahnaf-tahmid-chowdhury · 2024-02-27T11:53:05Z

src/dagmc/DagMC.hpp

+   * @brief Gets the angle between the normal of a specified surface and a ray
+   * from the ray origin in a specified direction.
+   *
+   * @param surf The surface with respect to which the angle is determined.
+   * @param xyz A 3-element array representing the coordinates of the point.
+   * @param angle[out] A 3-element array in which to store the determined angle.


Suggested change

* @brief Gets the angle between the normal of a specified surface and a ray

* from the ray origin in a specified direction.

*

* @param surf The surface with respect to which the angle is determined.

* @param xyz A 3-element array representing the coordinates of the point.

* @param angle[out] A 3-element array in which to store the determined angle.

* @brief Gets the normal vector of the surface at the specified point of

* intersection by a ray from the ray origin in a specified direction.

*

* @param surf The surface with respect to which the normal vector is determined.

* @param xyz A 3-element array representing the coordinates of the point of

* intersection.

* @param angle[out] A 3-element array in which to store the determined normal vector.

I don't expect to change the name of the output parameter in this PR since it's only meant to update comments/documentation.

Is this right?

gonuke

Sorry for this clang-format concern...

gonuke · 2024-02-27T17:01:26Z

src/dagmc/DagMC.hpp

+   * calculation. The search for that triangle can be circumvented by providing a
+   * RayHistory, in which case the last triangle of the history will be used.


It looks like our clang-format wants to wrap this line differently 🤷

Suggested change

* calculation. The search for that triangle can be circumvented by providing a

* RayHistory, in which case the last triangle of the history will be used.

* calculation. The search for that triangle can be circumvented by providing

* a RayHistory, in which case the last triangle of the history will be used.

No worries! Ran clang-format locally and pushed before I saw this. Sorry to not incorporate the suggestion!

ahnaf-tahmid-chowdhury

I think everything is fine now. Let's ask @gonuke to merge the changes.

gonuke

Thanks @pshriwise

pshriwise added the Type: Documentation & Install label Feb 26, 2024

pshriwise added 5 commits February 25, 2024 23:46

Adding documentation to DAGMC header. Needs review

8b041b2

Corrections/updates to the documentation

04ba4c4

Updating/adding inline documentation for the metadata class

72fd3f7

Applying style guide

0d6fe7e

Updating changelog

dabcd73

pshriwise force-pushed the interface-docs branch from bb038e0 to dabcd73 Compare February 26, 2024 05:47

gonuke requested changes Feb 26, 2024

View reviewed changes

src/dagmc/dagmcmetadata.hpp Show resolved Hide resolved

pshriwise commented Feb 26, 2024

View reviewed changes

src/dagmc/dagmcmetadata.hpp Show resolved Hide resolved

Remove redundant method signature line

e8656f1

ahnaf-tahmid-chowdhury reviewed Feb 26, 2024

View reviewed changes

gonuke requested changes Feb 26, 2024

View reviewed changes

pshriwise and others added 4 commits February 26, 2024 21:34

Addressing comments in PR review

b485a43

Adding docs for dagmcmetadata constructor and remove duplicat props

19b7faf

Apply suggestions from @ahnaf-tahmid-chowdhury

d97c300

Co-authored-by: Ahnaf Tahmid Chowdhury <[email protected]>

Adding more information on private parse_ methods

eb30c20

pshriwise commented Feb 27, 2024

View reviewed changes

Fix format

3142099

gonuke requested changes Feb 27, 2024

View reviewed changes

ahnaf-tahmid-chowdhury reviewed Feb 27, 2024

View reviewed changes

Improvements to get_angle docs and other small updates.

bc6241a

gonuke requested changes Feb 27, 2024

View reviewed changes

Style fixes

9a74ad6

ahnaf-tahmid-chowdhury approved these changes Feb 27, 2024

View reviewed changes

gonuke approved these changes Feb 27, 2024

View reviewed changes

gonuke merged commit 96eab05 into svalinn:develop Feb 27, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inline documentation for the DagMC and dagmcmetadata classes. #945

Add inline documentation for the DagMC and dagmcmetadata classes. #945

pshriwise commented Feb 26, 2024

gonuke left a comment

ahnaf-tahmid-chowdhury left a comment

ahnaf-tahmid-chowdhury Feb 26, 2024

gonuke left a comment

gonuke Feb 26, 2024

pshriwise Feb 27, 2024

gonuke Feb 26, 2024

pshriwise Feb 27, 2024

gonuke Feb 26, 2024

pshriwise Feb 27, 2024

gonuke Feb 26, 2024

pshriwise Feb 27, 2024

gonuke Feb 26, 2024

gonuke Feb 26, 2024

gonuke Feb 26, 2024

gonuke Feb 26, 2024

pshriwise Feb 27, 2024

gonuke Feb 26, 2024

pshriwise Feb 27, 2024

pshriwise Feb 27, 2024

gonuke left a comment

gonuke Feb 27, 2024

pshriwise Feb 27, 2024

ahnaf-tahmid-chowdhury Feb 27, 2024 •

edited

Loading

gonuke Feb 27, 2024

ahnaf-tahmid-chowdhury Feb 27, 2024

gonuke left a comment

gonuke Feb 27, 2024

pshriwise Feb 27, 2024

ahnaf-tahmid-chowdhury left a comment

gonuke left a comment

		* @param[out] result The result of the operation. If present and
		* non-empty, the history is used to look up the surface facet at which the

		* @brief Gets the angle between a specified surface and a ray from a given
		* point in a specified direction.

	* @param dagmc A pointer to the DagMC instance associated with this metadata.
	* @param DAGptr A pointer to the DagMC instance associated with this metadata.

-   * @brief Gets the angle between the normal of a specified surface and a ray
-   * from the ray origin in a specified direction.
-   *
-   * @param surf The surface with respect to which the angle is determined.
-   * @param xyz A 3-element array representing the coordinates of the point.
-   * @param angle[out] A 3-element array in which to store the determined angle.
+   * @brief Gets the normal vector of the surface at the specified point of
+   * intersection by a ray from the ray origin in a specified direction.
+   *
+   * @param surf The surface with respect to which the normal vector is determined.
+   * @param xyz A 3-element array representing the coordinates of the point of
+   * intersection.
+   * @param angle[out] A 3-element array in which to store the determined normal vector.

		* calculation. The search for that triangle can be circumvented by providing a
		* RayHistory, in which case the last triangle of the history will be used.

Add inline documentation for the DagMC and dagmcmetadata classes. #945

Add inline documentation for the DagMC and dagmcmetadata classes. #945

Conversation

pshriwise commented Feb 26, 2024

Description

gonuke left a comment

Choose a reason for hiding this comment

ahnaf-tahmid-chowdhury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnaf-tahmid-chowdhury Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnaf-tahmid-chowdhury left a comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

ahnaf-tahmid-chowdhury Feb 27, 2024 •

edited

Loading