28099 feat unittest assertclose

[jmag722]

Adding assertClose method to UnitTest, for comparing real/complex scalars and arrays against an expected value.

[jmag722]

@e-kayrakli I still need to rebase, but wanted your feedback on current progress so far, especially whether or not this is following best practices.

Logic-wise though, I required array shapes to match as assertEqual throws an error for that, figured we could stay consistent for now? Or we could allow 1D and multidimensional array comparisons if sizes match? Can't think of an application for that but let me know. Also, I throw a generic Error for mismatched shapes, should I create my own Error subclass?

Probably need more thorough tests/better documentation too.

[e-kayrakli]

Thanks @jmag722! On a quick look the direction looks right to me.

Just a few procedural caveats:

• I am working on something else (among other core devs) with a deadline on Monday, so, I'd rather give a more detailed feedback after I am past that.
• In case you missed it, we are preparing for the 2.7 release, which is due to come out in about ~2 weeks. As part of that, we have already frozen main for adding new features (working on bug fixes and doc updates for one more week or so). So, even if we can get the PR ready to merge, we can't merge it until we un-freeze main, which probably won't happen next week.

We can still work on the PR and I can test and help you test it next week. If we agree that it is ready to go, we can add a post-release label to it and I can merge it in once the release dust settles. In which case, this feature will be in version 2.8 due roughly in 3 months.

[jmag722]

Thanks! Oh good to know. There's no rush on this, we can talk more next week.

[jmag722]

@e-kayrakli I think it's in a decent spot, let me know what we can still improve before merging

[e-kayrakli]

Nice work! Thanks a lot!

I left some comments. Some are more clarification, some are relatively small changes in the code for better maintainability. I'd be up for running a test on the PR after those changes.

[e-kayrakli]

Thanks @jmag722 -- I resolved bunch of conversations based on your thumbs up. Though then realized that if you haven't made changes locally, you may need to unresolve them.

In any case, the conversations that are open -- you may resolve some of them without any action, some only requires superficial changes. Once you are done with changing the code, ping me again to get some testing going.

[e-kayrakli]

On the subject of testing: were you able to test the test you are adding? i.e.

> start_test test/library/packages/UnitTest/AssertClose/AssertCloseTest.chpl

in Chapel root. If you don't have start_test, you may need make test-venv first in $CHPL_HOME.

I will do the full test suite myself, but it would be good if you can make sure that the test you are adding indeed passes. It is unlikely that something else will fail in the testing suite given that this is strictly additive. But I'll make sure.

[jmag722]

Thanks @jmag722 -- I resolved bunch of conversations based on your thumbs up. Though then realized that if you haven't made changes locally, you may need to unresolve them.

In any case, the conversations that are open -- you may resolve some of them without any action, some only requires superficial changes. Once you are done with changing the code, ping me again to get some testing going.

current resolution status looks good, I'll work on the other things. I've never really done a legit pull request on a big codebase like this before, for these types of edits, do I just push additional changes and mark it resolved here? Or do I somehow reference your suggestions and it resolves them automatically?

[jmag722]

On the subject of testing: were you able to test the test you are adding? i.e.

> start_test test/library/packages/UnitTest/AssertClose/AssertCloseTest.chpl

in Chapel root. If you don't have start_test, you may need make test-venv first in $CHPL_HOME.

I will do the full test suite myself, but it would be good if you can make sure that the test you are adding indeed passes. It is unlikely that something else will fail in the testing suite given that this is strictly additive. But I'll make sure.

I've been running ./util/start_test test/library/packages/UnitTest/AssertClose --compopts --devel and it's been passing

[e-kayrakli]

for these types of edits, do I just push additional changes and mark it resolved here

This is what I do myself. We don't have any strict rules as to what "Resolve conversation" means. So, feel free to do whatever makes most sense for you. It can get unwieldy with too many unresolved conversations, so I am always eager to get them out of the way. But, as I said, it is your PR and I can adapt to different styles. :)

There is no automation for auto-resolving things as far as I know. I don't need resolution for each comment to be in a separate commit either. Do whatever is more comfortable for you.

I've been running ./util/start_test test/library/packages/UnitTest/AssertClose --compopts --devel and it's been passing

That's probably OK, but you shouldn't use --compopts --devel or anything of the sort. It should only impact the behavior on an error, so it is probably moot, but the rule of thumb is to test things as close to the most common setting users use, which means not using --devel. Consider that flag as more for debugging as the error messages you'll get will be more verbose and won't shy away from showing some details of the internal modules and the compiler. And as another caveat, --devel may slow down the performance, which is not relevant here, but could be in the future.

If chpl is available (i.e. in $PATH), and if you did make test-venv already, start_test should be available, too. Though I am not aware of any drawback of doing ./util/start_test either. Just wanted to let you know for convenience.

[jmag722]

ok I've redone the logic on NaN, it is much more consistent now (allowing partial NaN comparison for complex numbers, nan+0i == nan+0i now), I feel better about that. code got a little more verbose in withinTol because generic procedure cannot promote functions that have short-circuiting logical operators (&&, ||). If you see any tricks to clean that up a bit let me know, I used nested procedures to keep the those helpers private. Also reorganized the tests and made them easier to read.

[e-kayrakli]

Thanks! I just posted two superficial comments. I'll do some testing without waiting for you to address those. Code-wise this looks good to me!

[e-kayrakli]

Full linux64 testing passed.

@jmag722 -- could you make sure to address the minor feedback I have? It'd be best if you do not rebase so I can just see the new commits with small changes once you have it. As long as there are no code changes in those new commits (not needed per my feedback), I can just merge it in without re-running testing.

Thanks again!

[jmag722]

@e-kayrakli just in case you didn't see, I did make minor code edits to the logic itself

[e-kayrakli]

The testing still looks clean! Let's freeze the code so I can merge it as soon as I can. The only thing that's missing is DCO. We need you to sign off each commit you have. The documentation has some details on it here.

The final part of that page recommends a solution to update an already existing branch. I find the solution there to be a bit roundabout. I am in the process of updating the documentation (#28356 if curious). But I think running:

     git rebase --signoff $(git merge-base HEAD upstream/main)
     git push origin --force-with-lease

is the solution here. The first commit will rebase your branch on where you started (practically speaking, it won't create any new merge/logic conflicts. It will add the signoff message to each commit. The second commit will force push your changes. Once you rebase, you can't do regular push, but you need a force push. It will impact my local copy I used for testing, but it doesn't matter as I don't intend to use that branch any further.

Once you fix that, CI will proceed to actual testing. There are several others after the DCO check. It would be very surprising if any of those fail though, given that you are not changing anything core to the code base here.

[e-kayrakli]

Note that we try not to merge code on Fridays unless we are planning to work over the weekend (I am not), because of the off chance that it may break nightly tests going to the weekend. So, it is very likely the earliest I can merge this is next Monday, unless you quickly fix the DCO, which is not needed for my sake. :)