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 mechanics for retrieving filter properties. #2693

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

phlptp
Copy link
Member

@phlptp phlptp commented Nov 11, 2024

Summary

If merged this pull request will add mechanics for getting filter properties

Proposed changes

@phlptp
Copy link
Member Author

phlptp commented Nov 11, 2024

will add the C version of this soon to this PR

@phlptp
Copy link
Member Author

phlptp commented Nov 11, 2024

Fixes #2669

@trevorhardy
Copy link
Contributor

@phlptp are you wanting a review on this?

@phlptp
Copy link
Member Author

phlptp commented Nov 11, 2024

@phlptp are you wanting a review on this?

Yes, Mainly to make sure it meets your needs regarding #2669

Copy link

@josephmckinsey josephmckinsey left a comment

Choose a reason for hiding this comment

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

It would be nice to return more meaningful errors.

void FilterOperations::set(std::string_view /*property*/, double /*val*/) {}
void FilterOperations::setString(std::string_view /*property*/, std::string_view /*val*/) {}

double FilterOperations::getProperty(std::string_view /*property*/)
{
return invalidDouble;

Choose a reason for hiding this comment

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

Any reason this shouldn't just error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we have to do a bunch of error checking through a lot of calls.

Choose a reason for hiding this comment

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

I'm not a big fan of reusing two doubles named param1 and param2, and this definitely goes against the spirit of the linter here. Any way we can use a tagged union or std::variant instead? Might be helpful in the if statements too.

Copy link
Member Author

Choose a reason for hiding this comment

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

the parameters have different meanings depending on which random distribution is used, so there is no standard naming convention there.

@@ -273,13 +350,45 @@ void RerouteFilterOperation::setString(std::string_view property, std::string_vi
}
catch (const std::regex_error& re) {
std::cerr << "filter expression is not a valid Regular expression " << re.what()
<< std::endl;
<< '\n';

Choose a reason for hiding this comment

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

Is this to avoid flushing cerr?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think cerr is flushed anyway, so I don't think the flushing matters here.

@trevorhardy
Copy link
Contributor

@josephmckinsey, can you get the C-based additions @phlptp just added into PyHELICS for testing? I don't know if we want to use the libclang script or do it manually. Maybe @nightlark has an idea on the best way to do this?

@nightlark
Copy link
Member

For pyhelics changes: use https://github.com/GMLC-TDC/cheader2json#usage to dump ast.json files for the previous helics release (using the helics.h backup header file) and for the new development version of helics (using the new helics.h backup header file), then use it to do a diff of the ast.json files to get the changes that need to be made to pyhelics.


/** get a property from a filter
@param property the name of the property of the filter to change
@param val the numerical value of the property
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
@param val the numerical value of the property

@@ -51,6 +51,19 @@ class FilterOperations {
@param val the numerical value of the property
*/
virtual void setString(std::string_view property, std::string_view val);

/** get a property from a filter
@param property the name of the property of the filter to change
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
@param property the name of the property of the filter to change
@param property the name of the property of the filter to get

*/
virtual double getProperty(std::string_view property);
/** get a string property on a filter
@param property the name of the property of the filter to change
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
@param property the name of the property of the filter to change
@param property the name of the property of the filter to get

virtual double getProperty(std::string_view property);
/** get a string property on a filter
@param property the name of the property of the filter to change
@param val the numerical value of the property
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
@param val the numerical value of the property

@@ -84,6 +84,17 @@ class HELICS_CXX_EXPORT Filter: public Interface {
*/
virtual void setString(std::string_view property, std::string_view val);

/** get a string property on a filter
@param property the name of the property of the filter to change
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
@param property the name of the property of the filter to change
@param property the name of the property of the filter to get

* Get a double property from a filter
*
* @param filt The filter to modify.
* @param prop A string containing the property to set.
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
* @param prop A string containing the property to set.
* @param prop A string containing the property to get.

* Set a string property on a filter. The string output memory is valid until a subsequent call to to getPropertyString on the particular
filter
*
* @param filt The filter to modify.
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
* @param filt The filter to modify.
* @param filt The filter to retrieve a value from.

filter
*
* @param filt The filter to modify.
* @param prop A string containing the property to set.
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
* @param prop A string containing the property to set.
* @param prop A string containing the property to get.

HELICS_EXPORT double helicsFilterGetPropertyDouble(HelicsFilter filt, const char* prop);

/**
* Set a string property on a filter. The string output memory is valid until a subsequent call to to getPropertyString on the particular
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
* Set a string property on a filter. The string output memory is valid until a subsequent call to to getPropertyString on the particular
* Get a string property on a filter. The string output memory is valid until a subsequent call to getPropertyString on the particular


/**
* Set a string property on a filter. The string output memory is valid until a subsequent call to to getPropertyString on the particular
filter
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
filter
filter.

@trevorhardy
Copy link
Contributor

For pyhelics changes: use https://github.com/GMLC-TDC/cheader2json#usage to dump ast.json files for the previous helics release (using the helics.h backup header file)...

Any update on getting these changes into PyHELICS for testing?

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