Skip to content

Correct Units.pm - value of the electron volt#689

Closed
duffee wants to merge 2 commits intoopenwebwork:PG-2.17from
duffee:Units_correct_electronvolt
Closed

Correct Units.pm - value of the electron volt#689
duffee wants to merge 2 commits intoopenwebwork:PG-2.17from
duffee:Units_correct_electronvolt

Conversation

@duffee
Copy link
Contributor

@duffee duffee commented Jun 2, 2022

This PR corrects the exponential for factor of eV in lib/Units.pm, which was too large by a factor 10^10.
fixes #677

I've taken the opportunity to add definitions for keV, MeV and GeV. These units are used in particle physics and for gamma ray energies. The indentation in that section was tidied.

I've added to t/README.md and removed the redundant t/README
I've also added unit test files for Units.pm and parserNumberWithUnits.pl. I've found that the Test2 suite is a good framework for writing tests. To make installing module dependencies easier, I've included a cpanfile and instructions for different use cases.

The warnings about subroutine redefinitions in t/contexts/trig_degrees.t have been silenced.

This PR supersedes #678

duffee added 2 commits May 31, 2022 13:55
Correct exponential for factor of eV
fixes openwebwork#677

Adds definitions for kev, MeV and GeV
Adds unit tests for Units.pm and test framework for parserNumberWithUnits.pl

Adds cpanfile for external module dependencies

Updates t/README.md with description of integration tests,
unit tests, instructions for how to run these tests
and how to use the cpanfile
Remove t/README because it has been superseded by t/README.md
When testing trig functions, the subs are redefined
and emit warnings.  The functions are now removed from
the symbol table before the PG macro contextTrigDegrees.pl
is loaded.
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

I am sorry, but I cannot approve this. We are already using the Test::More module, and we do not need another module. Either the existing tests should be switched to using Test2::V0, or these tests need to be switched to Test::More.

I still do not agree with the addition of the cpanfile either.

@duffee
Copy link
Contributor Author

duffee commented Jun 2, 2022

Can I migrate the existing tests to Test2 in a separate PR? I'm happy to do the work, but it's not my day job so it could take a week to turn around.

Reasons to keep the cpanfile:

  • it's small
  • you're not forced to use it
  • its presence doesn't affect any other workflow
  • it's very useful. It makes installing dependencies easy as cpanm --installdeps ., it allows the separation of concerns (run time/test/develop), it lets you specify which versions are required or conflict (e.g. if you didn't have an easy resolution to the SQL::Abstract 2.000001 bug, you could say not to install that particular version)
  • it's now a standard (appeal to authority here) My last 2 workplaces have required that any new modules used get added to the cpanfile.

Do you have a reason for excluding it, besides trying to keep the codebase small?

@drgrice1
Copy link
Member

drgrice1 commented Jun 2, 2022

It is an unnecessary file.

Installing perl modules using cpan or cpanm is not the ideal way to install perl modules on many linux distributions. It taints the system with files that are not tracked by the distributions package management system. Ideally all perl modules would be installed via the distribution's package management system.

There is a resolution to the SQL::Abstract 2.0000001 bug. Install SQL::Abstract::Classic.

@drgrice1
Copy link
Member

drgrice1 commented Jun 3, 2022

I think that this should be retargeted to the develop branch. I don't think that this is ready for the release.

I also think that you should collaborate with @pstaabp on which test module to use. Both Test2::V0 and Test::More are full featured test suites.

@pstaabp
Copy link
Member

pstaabp commented Jun 3, 2022

@duffee I did look into the Test2::V0 module briefly, but is there some compelling feature to use that over Test::More?

The cpanfile seems like a good idea to me. It seems like for those who don't use distributions to install perl modules, it is an easy way to pull in all modules with cpanm. And as @duffee mentions, it's one small file.

@duffee
Copy link
Contributor Author

duffee commented Jun 4, 2022

@pstaabp Test2 is more compact. It can test nested data structures with a smaller, neater description of the data; it understands that hash keys are randomly ordered; it pulls in the ecosystem of testing tools for warnings, exceptions, mocks and classes into one suite. I was told that it could run the subtests in random order to flush out tests that depend on state but I haven't been able to verify that recently. It's advantage is that it takes the 15 years of experience with testing and make the changes in the background that would break Test::More. It's close enough that migrating from Test::More isn't too onerous (they are mostly drop-in replacements). It is developed by the author of Test::More as the path for future development.

The places I've worked have large test suites that take time to migrate, so the ideal is that new tests are written in Test2, if convenient, and eventually we'll re-write the older tests. The PG suite is still quite small and I'm happy to do the work against the develop branch and they can be merged in one self-consistent PR. I was used to working in a mixed Test::More/Test2 environment and I wasn't comfortable with changing all your tests (since you're the only other person to write unit tests for PG) without having the conversation first.

I'll spin up a new PR for the tests and start to migrate some of your tests so you can see from the diffs what could be gained or not.

@duffee
Copy link
Contributor Author

duffee commented Jun 6, 2022

This PR has been split into #690 #691 and #692 and the discussions here have been copied across.

@duffee duffee closed this Jun 6, 2022
@duffee duffee deleted the Units_correct_electronvolt branch June 9, 2022 10:22
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.

3 participants