Reinventing SiteRDF

[jws-1]

This PR implements the SiteRDF module outside of the Procedure framework (similarly to #1747), and works towards #1769. I believe this is working as intended, but will need a more scientific eye to confirm!

As a side, this PR also implements a DataNormaliser1D class for future analysis, with intentions of performing similarly to the operate...Normalise family of nodes.

Some questions before merging can be performed:

• In the previous implementation, it seems that the instantaneous_ flag is not actually used - is this intentional? If not, then I can update the new analysis code to utilise it.
• Currently, the DataNormaliser1D methods that operate on Site-based statistics just take the site population as input - which seems OK for now, but is there a better solution that would be more useful in the long term? Also, would it be better to have a DataNormaliserBase, which DataNormaliser1D inherits from, or should we wait until 2/3D variants are needed?
• I've updated some of the SiteRDF test input files to enable the excludeSameMolecule flag, as it felt like it should be set in some places - is this correct? If not, there is a bigger problem at hand as disabling it causes the tests to fail!

[trisyoungs]

Excellent. This looks really good - I'm already fully convinced of this new way of writing analysis routines!

Some small comments in the review, and I'll address your questions. I'd like to do some local testing of the module before we merge, particularly the instantaneous option.

[trisyoungs]

Answers to your questions above:

1. Instantaneous - yes, you're right. This was left over from when we calculated coordination numbers in the module, so it can be removed in this implementation. It may get reintroduced properly at a later date.

2. DataNormaliser1D - I think what you've done here is the best route for now. We don't need the class to be very clever, so just taking pure numbers as input is fine. Probably also no need to have a base class since we're using it in code only for now.

3. excludeSameMolecule flag - again, I believe you're right. In fact I'm surprised that it wasn't enabled by default in the module itself. Hey ho!

[jws-1]

1. Instantaneous - yes, you're right. This was left over from when we calculated coordination numbers in the module, so it can be removed in this implementation. It may get reintroduced properly at a later date.

I'm a tad confused! Do we not still calculate coordination numbers in this module?

[trisyoungs]

1. Instantaneous - yes, you're right. This was left over from when we calculated coordination numbers in the module, so it can be removed in this implementation. It may get reintroduced properly at a later date.

I'm a tad confused! Do we not still calculate coordination numbers in this module?

Hah. Not as confused as me apparently! The CN calculation is still there, of course!

Incidentally we have been talking about recombining HistogramCN into SiteRDF since it kinda makes sense to do so, but we can leave that as an additional refactoring afterwards (I see you have #1788 in).

[trisyoungs]

Further to this, I just did a crude benchmark of the old vs new versions of the module and the gains are...... modest to say the least, with the old version clocking at 0.007 s/iter on my machine vs 0.0068 (at best) for the new. We can take positives in the fact that the old node-base system wasn't a complete overhead penalty, and the fact that we can easily go back in after all the modules are refactored and implement true parallelism.