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

Add docstring and oop array construction method for build_explicit_observed_function #3200

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

BenChung
Copy link
Contributor

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.

@BenChung BenChung force-pushed the explicit-observed-func branch 2 times, most recently from e81ac30 to e91738b Compare November 13, 2024 00:46
@ChrisRackauckas
Copy link
Member

Can we translate this to the standard? https://github.com/SciML/SciMLStyle?tab=readme-ov-file#documentation we should update that to use ## for the sections but still, the point being that it documents it by arg is a bit easier to read. I don't quite understand how it's written here.

@BenChung
Copy link
Contributor Author

This one is tricky because it's substantially specifying the signature of a method it returns, rather than the method itself. I'm not sure where in the style guide it talks about documenting the argument and return types of returned function values? This is also an issue for SII, too, so it would be worthwhile coming up with a standard.

* `checkbounds = true` checks bounds if true when destructuring parameters
* `op = Operator` sets the recursion terminator for the walk done by `vars` to identify the variables that appear in `ts`. See the documentation for `vars` for more detail.
* `throw = true` if true, throw an error when generating a function for `ts` that reference variables that do not exist
* `drop_expr` is deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

why would it be deprecated? How else would you control the rgf return serialization?

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; it's not actually used for anything in the function so far as I can tell.

That statement comes from Fredrik:

drop_expr is no longer needed, it was used to control whether or not the Expr was included in the generated function or not, for debugging purposes

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to just always require RuntimeGeneratedFunctions.get_expression yes, but then if this argument is never documented or used, why mention it in the docstring at all? Just remove it?

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 think that when I wrote this I was just trying to document each argument... yeah, it should probably just be deleted. It looks like the only other use is

drop_expr = identity)
, which is doing exactly nothing.

@BenChung
Copy link
Contributor Author

@ChrisRackauckas What would help a lot is if there's an example/a standard for how to document first-class functions, since that's ultimately what's being described here (albeit sort of in a matrix of the argument configuration to the signature of the output function). I couldn't find it in the style guide you linked, but I might just be missing something.

The proximal problem is that the function (and the functions it returns) behaves very differently depending on the arguments and the system that it's generating an observed function for. It can return one or two functions, and the signatures of the returned functions can have between 0 and 5 arguments depending on a bunch of different properties.

@ChrisRackauckas
Copy link
Member

Rebase

@BenChung BenChung force-pushed the explicit-observed-func branch from c6e14c0 to 14996d7 Compare November 15, 2024 01:59
@ChrisRackauckas ChrisRackauckas merged commit 2c19234 into SciML:master Nov 15, 2024
31 of 39 checks passed
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