Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alias tempfactor & bfactors #1901

Closed
richardjgowers opened this issue May 16, 2018 · 8 comments · Fixed by #3345
Closed

Alias tempfactor & bfactors #1901

richardjgowers opened this issue May 16, 2018 · 8 comments · Fixed by #3345

Comments

@richardjgowers
Copy link
Member

These are currently separate attributes, but are in fact the same thing. Because both names are widely used maybe it makes more sense to alias them, so a single TopologyAttr gives you both access points...

@orbeckst
Copy link
Member

I am not 100% sure what the best way forward is (even though I raised the problem in #1899 (comment)). I am not a huge fan of lots of special casing if it can be avoided (and the Python philosophy is to "there should be preferable one way to do something"). Perhaps just the PDBWriter should take either and fail if both are present? Then the special casing is restricted to only one small part of the code and the user experience?

(And then we can alias it to beta, too.)

@orbeckst
Copy link
Member

orbeckst commented Nov 2, 2018

There's also an issue that MMTFReader used bfactors and PDBReader/Writer uses tempfactors. This leads to the annoying situation that writing a PDB from MMTF looses the tempfactors:

import MDAnalysis as mda
u = mda.fetch_mmtf('1AKE')
u.select_atoms("segid A").write("1AKE_A.pdb")

gives

~/anaconda3/lib/python3.5/site-packages/MDAnalysis/coordinates/PDB.py:906: UserWarning: Found no information for attr: 'tempfactors' Using default value of '0.0'
  "".format(attrname, default))

@orbeckst
Copy link
Member

orbeckst commented Nov 2, 2018

Workaround for the MMTF/PDB case:

import MDAnalysis as mda
u = mda.fetch_mmtf('1AKE')
u.add_TopologyAttr('tempfactors', u.atoms.bfactors)

@IAlibay
Copy link
Member

IAlibay commented Mar 11, 2021

@lilyminium and I are keen to see this get fixed for 2.0, if anything it breaks colour-by-bfactor visualisation in NGLView.

Do we need to warn users in 1.0.2? (I don't think so, but maybe I'm missing something).

@lilyminium
Copy link
Member

lilyminium commented Mar 11, 2021

To repeat what I said on Discord, IMO we should in 2.0 but maybe not 1.0.2. My argument is that users may have assigned different information to bfactors and tempfactors in existing code. The most likely scenario to select atoms with custom tags (e.g. "bfactor 2" vs "tempfactor 2")

IAlibay pushed a commit that referenced this issue Mar 14, 2021
…actors (#3161)

Towards #1901

## Work done in this PR
- Adds a warning when trying to add_TopologyAttr for bfactors or tempfactors when the other already exists.
@orbeckst
Copy link
Member

Note that 1.1.0 promised 1eede18 :

In version 2.0, MDAnalysis will stop treatiing tempfactors and bfactors as separate attributes. Instead, they will be aliases of the same attribute.

If we decide to not honor this promise we would technically have to do 1.2.0 with a deprecation note. (We can decide that we're breaking semver a little bit here — but at least we should be clear about what we're doing.)

@IAlibay
Copy link
Member

IAlibay commented May 28, 2021

Note that 1.1.0 promised 1eede18 :

In version 2.0, MDAnalysis will stop treatiing tempfactors and bfactors as separate attributes. Instead, they will be aliases of the same attribute.

If we decide to not honor this promise we would technically have to do 1.2.0 with a deprecation note. (We can decide that we're breaking semver a little bit here — but at least we should be clear about what we're doing.)

🙀 Please no more 1.x branch - @lilyminium you're our only hope.

@lilyminium
Copy link
Member

To sum up earlier discussion before we all forget it, we have decided to alias bfactor to tempfactor so that tempfactor is the One True TopologyAttr.

IAlibay pushed a commit that referenced this issue Aug 12, 2021
Fixes #1901 

## Work done in this PR
- Aliases bfactor to tempfactor
- Adds deprecation warnings for bfactor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants