Skip to content

Conversation

@andrsd
Copy link
Collaborator

@andrsd andrsd commented Jan 2, 2024

This introduces the Function system with following ideas:

  • There is a C++ base class for a "function call" that is used in the core of OpenSn
    • the LUA layer inherits from this base class and calls the LUA interpreter with the supplied function name (this preserves the functionality so that users can define their own functions in the LUA script and have those used by the core C++ classes). This is what was done via CallLua???Function et al.
    • application code can use this base class to define its own function and have it called (in future, things like parsed functions can be created by inheriting from this base class)
  • Functions are stored in their dedicated function_stack.
  • The "functions" used by the core C++ classes are no longer stored by their names (which was possible when we assumed the use of just LUA functions), but they are stored as std::shared_ptr to an appropriate Function via a C++ API.

Refs #9

Copy link
Collaborator

@zhardy-lanl zhardy-lanl left a comment

Choose a reason for hiding this comment

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

  • Quite a few repetitive comments.
  • I think we should discuss the ResponseFunction classes. I think we should use a more general name. There can be many different scenarios where a group-wise function of position and material ID are used that are not response evaluations. It would naturally fit within a SpatialMaterialFunction and could be combined with ScalarSpatialMaterialFunction by changing the argument ordering and using num_components=1 default.
  • I am still quite confused about the val argument of ScalarMaterialFunction

@zhardy-lanl zhardy-lanl mentioned this pull request Jan 3, 2024
2 tasks
@andrsd
Copy link
Collaborator Author

andrsd commented Jan 8, 2024

I think we should discuss the ResponseFunction classes. I think we should use a more general name. There can be many different scenarios where a group-wise function of position and material ID are used that are not response evaluations. It would naturally fit within a SpatialMaterialFunction and could be combined with ScalarSpatialMaterialFunction by changing the argument ordering and using num_components=1 default.

Do you want to solve this in this PR (i.e. I need to do more work here) or in a new issue/PR? The main difference between Scalar???Functions and ResponseFunction right now is that scalar function return a scalar (hence the name) and ResponseFunction return a std::vector<double>. So there is definitely a room for more design to happen in future. Things that need to/should be decided on are like: should the API in the function system always return vector of values (single value functions could be seen as degenerate case that returns a single value) or should there be split or should there be Evaluate and VectorEvaluate (cannot have overloaded virtual methods that differ just in a return type).

I am just making sure if this PR is ready...

Copy link
Collaborator

@zhardy-lanl zhardy-lanl left a comment

Choose a reason for hiding this comment

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

Opened #68 for the response function discussion. This is good with me.

@andrsd
Copy link
Collaborator Author

andrsd commented Jan 8, 2024

IDK what happened for the first time, but the regression test workflow took 63 minutes and did not finish. I re-run it and it finished in an expected time. 🤔

@zhardy-lanl
Copy link
Collaborator

IDK what happened for the first time, but the regression test workflow took 63 minutes and did not finish. I re-run it and it finished in an expected time. 🤔

Unless anything needs to be done with respect to that, feel free to merge this.

Copy link
Collaborator

@wdhawkins wdhawkins left a comment

Choose a reason for hiding this comment

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

I like this. Clean, straightforward, easy to understand. I'm not overly opposed to the ResponseFunction naming. I can understand the argument for changing it. Maybe EvalFunction or something. I don't think I have strong opinions either way.

@andrsd andrsd merged commit 9f4c021 into Open-Sn:main Jan 8, 2024
@andrsd andrsd deleted the function-system branch January 8, 2024 21:55
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