hoomd-blue icon indicating copy to clipboard operation
hoomd-blue copied to clipboard

Remove diameter.

Open joaander opened this issue 3 years ago • 3 comments

Description

Remove the diameter shifting logic in all C++ neighbor list implementations.

Motivation and context

With the removal of SLJ and related methods in v3, this code is no longer needed.

joaander avatar Mar 04 '22 19:03 joaander

Includes:

  • Removing diameter shifting in the neighbor list.
  • Removing diameter from snapshot data structures.
  • Modifying the built in GSD writer and reader to ignore diameters.
  • Possibly removing the diameter particle property (though this is a lot of work with little to gain).
  • Find and remove references to diameter in the documentation.

Reason: Diameters cause more confusion that they help when working with visualization tools like OVITO. Many users also make assumptions that HOOMD uses the diameter in some way when it does not.

joaander avatar Aug 12 '22 14:08 joaander

Strongly support, diameter shifting triggers some exotic and little used code paths in the neighbor list. I think there might also be some code floating around in the communicator related to this too, but I don't remember if/how @jglaser might have refactored some of that. Suggest to add:

  • Check Communicator and CommunicatorGPU for diameter shifting in MPI and remove as needed

to your list! I think you might need to be careful doing this part, though, because there may be some stuff in the that looks like diameter shifting but is actually related to rigid body diameters rather than particle diameters.

mphoward avatar Aug 12 '22 14:08 mphoward

Will do.

joaander avatar Aug 12 '22 15:08 joaander

I have a bit of a side comment to this. The presence of the diameter property was practical for custom evaluators. By abusing this property, one could write a potential that depended on a per-particle property. It's still possible to do without, but it requires a significant amount of code, for instance new force compute kernel, need to connect to particle sorting signal, etc.

It might be useful to also provide a method to create custom evaluator that require custom per-particle properties when diameters are removed.

MartinGirard avatar Oct 18 '22 15:10 MartinGirard

I have a bit of a side comment to this. The presence of the diameter property was practical for custom evaluators. By abusing this property, one could write a potential that depended on a per-particle property. It's still possible to do without, but it requires a significant amount of code, for instance new force compute kernel, need to connect to particle sorting signal, etc.

It might be useful to also provide a method to create custom evaluator that require custom per-particle properties when diameters are removed.

Do you need both custom per-particle quantities and charge? If not, you could abuse charge instead of diameter.

I agree that the ideal solution would be a system that supports dynamically added and arbitrarily named per-particle quantities. That is a very complex feature to introduce. I would hope to add it some day but I don't have a time frame.

joaander avatar Oct 18 '22 15:10 joaander

I have a bit of a side comment to this. The presence of the diameter property was practical for custom evaluators. By abusing this property, one could write a potential that depended on a per-particle property. It's still possible to do without, but it requires a significant amount of code, for instance new force compute kernel, need to connect to particle sorting signal, etc. It might be useful to also provide a method to create custom evaluator that require custom per-particle properties when diameters are removed.

Do you need both custom per-particle quantities and charge? If not, you could abuse charge instead of diameter.

I agree that the ideal solution would be a system that supports dynamically added and arbitrarily named per-particle quantities. That is a very complex feature to introduce. I would hope to add it some day but I don't have a time frame.

There's many situations where I could see both used. The P3M method still requires charges, and if one wants a potential that abuses diameter plus long range forces, then it requires both. It's still very hacky since you can easily get conflicts if two potentials abuse the same quantity.

MartinGirard avatar Oct 18 '22 17:10 MartinGirard

@MartinGirard do you need to read/write diameters from GSD files when using them as ad-hoc per-particle parameters?

I ask because part of the motivation for this issue is to remove diameter from the GSD file to give users better control over visualization in OVITO. If you don't need this functionality, we could remove diameter from GSD but keep it as an unused particle quantity in HOOMD at least until we someday provide for custom per-particle quantities created on demand.

joaander avatar Oct 20 '22 15:10 joaander

@MartinGirard do you need to read/write diameters from GSD files when using them as ad-hoc per-particle parameters?

I ask because part of the motivation for this issue is to remove diameter from the GSD file to give users better control over visualization in OVITO. If you don't need this functionality, we could remove diameter from GSD but keep it as an unused particle quantity in HOOMD at least until we someday provide for custom per-particle quantities created on demand.

In some cases they do. We've been working on one where "diameters" were coupled to MC moves. If it's for GSD export, the property doesn't really need to be called "diameter", could be simply "extra" (or something more descriptive).

MartinGirard avatar Oct 20 '22 16:10 MartinGirard

Well, maybe we can work around this by requiring an opt-in to write diameters in v4.

joaander avatar Oct 20 '22 17:10 joaander

When implementing this, convert enable_shared_cache in the neighbor list to a template parameters. When profiling, I recently found that this branch is predicated, so both sides of the branch are always run: https://github.com/glotzerlab/hoomd-blue/blob/3539c5ea43502b184a7a02dbdc42524ab1fcf08b/hoomd/md/NeighborListGPUBinned.cu#L230-L239

Make this change when removing diameters, as removing the diameter_shift template parameter while adding enable_shared_cache keeps the number of kernels built constant.

joaander avatar Nov 30 '22 19:11 joaander

Fixed in #1532.

joaander avatar Apr 13 '23 19:04 joaander