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

feat: add relative equals that defaults to direct equals for small values #303

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

Conversation

kurtsansom
Copy link
Contributor

@kurtsansom kurtsansom commented May 12, 2021

For relative equals there is check in case the values are small.

This commit provides a relative equals that defaults to direct when the values are small.
e.g. max(relative_tolerance*abs(expected), tolerance)

If there is a better way to do this please advise.

Originally I tried to keep with the naming convention, but ran into trouble with the length of function names and therefore used RelMinEqual which I really don't like.

Another thing that need some more attention is I didn't modify the added test yet to evaluate the threshold. It would be good to get feedback that this is the right approach before doing more.

Copy link
Member

@tclune tclune left a comment

Choose a reason for hiding this comment

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

I'll await feedback on my comments.

Also note that when I spoke to the lawyer on Friday, she indicated that no CLA's have yet been submitted for pFUnit. (Am guessing it is still in the hands of your institutions laywers.) Just note that I cannot merge the PR until the lawyer on my end gives the green light. (sigh)

src/funit/asserts/Assert_Real.tmpl Outdated Show resolved Hide resolved
@kurtsansom
Copy link
Contributor Author

I decided to rename the function to approx to simplify the name length, and mirror the name use by pytest as there doesn't appear to be an equivalent in JUnit. I removed the relative difference calculation as providing the difference and calculated tolerance is sufficient to describe the criteria that is failing.

@tclune
Copy link
Member

tclune commented May 17, 2021

Looks like it would not be too difficult to introduce aliases, but I'd not ask you to implement it.

The intro would define a small dictionary whose keys are the annotations and the values are the Fortran procedure names. It would then intercept the usual expansion:

        parser.outputFile.write(fragment.format(match.group(1), match.group(2)))

@kurtsansom
Copy link
Contributor Author

I made a simple example on this branch.

https://github.com/kurtsansom/pFUnit/tree/assert_alias

map:

assert_alias_map = {'approx' : 'ApproxEqual'}

in class AtAssert(Action):

        if (match.group(1).lower() in self.alias_map.keys()):
            function_name = self.alias_map[match.group(1).lower()]
        else:
            function_name = match.group(1)

@tclune
Copy link
Member

tclune commented Jul 5, 2021

Same request as the other. Please change to PR onto develop branch.

@kurtsansom
Copy link
Contributor Author

I will shoot for getting to this by the end of the week.

@tclune tclune self-requested a review July 23, 2021 12:01
@tclune
Copy link
Member

tclune commented Nov 15, 2021

@kurtsansom - do you still intend to resubmit this as a PR onto develop?

@kurtsansom
Copy link
Contributor Author

kurtsansom commented Nov 23, 2021

@tclune Apologies this has fallen off my radar. It looks like I just missed fixing this by about a week.

@kurtsansom kurtsansom changed the base branch from main to develop November 23, 2021 16:43
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