Skip to content

Easyblock nwchem#441

Merged
boegel merged 7 commits intoeasybuilders:developfrom
hajgato:easyblock_nwchem
Sep 6, 2014
Merged

Easyblock nwchem#441
boegel merged 7 commits intoeasybuilders:developfrom
hajgato:easyblock_nwchem

Conversation

@hajgato
Copy link
Copy Markdown
Collaborator

@hajgato hajgato commented Jul 14, 2014

Needed for NWChem version 6.3

@hpcugentbot
Copy link
Copy Markdown

Automatic reply from Jenkins: Can I test this?

Comment thread easybuild/easyblocks/n/nwchem.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You don't want to run tests for all versions below 6.3?

@wpoely86
Copy link
Copy Markdown
Member

ok to test

Comment thread easybuild/easyblocks/n/nwchem.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this will have to be done cleaner/shorter, but also: why is this needed? any references for this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Well, I looked into some of those examples file, and they will always fail (at lest with version 6.3), so why should we run them? And I am quite sure, that some of the tests were never run correctly with the previous versions (although I did not check them). For example, the (o)h3tr{3,4,5}.nw must run separately, and only (o)htr{1,2} must run sequentially. (Actually, the (o)h3tr2.nw is a restart of (o)h3tr1.nw. I could leave all the examples there but they will fail.

Honestly speaking, the examples in some case are outdated, and sometimes are not even implemented. Better way is exist to make the test, but the results should be always manually re-checked, beacuse we might have some unsignificant digits changed, or some numerical value orders are changed.

I can leave out the deletion part of the tests, but then it is not surprising that we have to set a limit for passed tests 50% or so.

Should I run all the tests then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good to hear you dove into these test, I was never able to (I lack the domain knowledge, for one).

Rather than removed these tests for a particular version of NWChem, just removed them all together from the list of tests being run, especially if you have serious doubt whether they ever worked at all.
Just add a comment with a list of names of tests that were omitted because they're known to be broken.
And feel free to reorder the dirdyvtst/oh3 and dirdyvtst/h3 tests to make sure they can actually succeed, no need to do that in a separate if clause either...

@hajgato
Copy link
Copy Markdown
Collaborator Author

hajgato commented Aug 4, 2014

Examples/tests fixed

@boegel
Copy link
Copy Markdown
Member

boegel commented Sep 6, 2014

Tested with easybuilders/easybuild-easyconfigs#969 and existing NWChem easyconfigs, looking great.

Thanks @hajgato!

boegel added a commit that referenced this pull request Sep 6, 2014
@boegel boegel merged commit d7a0d86 into easybuilders:develop Sep 6, 2014
@hajgato hajgato deleted the easyblock_nwchem branch October 16, 2014 08:53
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