glotzerlab / hoomd-blue

Molecular dynamics and Monte Carlo soft matter simulation on GPUs.
http://glotzerlab.engin.umich.edu/hoomd-blue
BSD 3-Clause "New" or "Revised" License
329 stars 127 forks source link

Remove diameter. #1266

Closed joaander closed 1 year ago

joaander commented 2 years ago

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 commented 2 years ago

Includes:

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.

mphoward commented 2 years ago

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:

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.

joaander commented 2 years ago

Will do.

MartinGirard commented 1 year ago

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.

joaander commented 1 year ago

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.

MartinGirard commented 1 year ago

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.

joaander commented 1 year ago

@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.

MartinGirard commented 1 year ago

@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).

joaander commented 1 year ago

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

joaander commented 1 year ago

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 commented 1 year ago

Fixed in #1532.