vvoelz / biceps

Bayesian inference of conformational populations
https://github.com/vvoelz/biceps
Other
12 stars 3 forks source link

Change experimental observable names variables as short as possible #27

Closed yunhuige closed 6 years ago

yunhuige commented 6 years ago

I feel it may be more easily to have a typo when we have too long names like protctionfactors, chemicalshifts, etc. How about change them to pf, cs, etc? @robraddi

robraddi commented 6 years ago

@yunhuige Sure, here's a few lines of code that should do the trick...

$  pwd
.../BICePs_2.0/restraint_edit/src
$  f=$(ls|grep .py)
$  for i in $f;do cat $i | sed 's/[old]/[new]/g' > $i_.py;done

We can specify numerous substitution commands to swap everything we need to change. Let's perform the swap after we merge our work.

yunhuige commented 6 years ago

@robraddi I think we need to be careful when we use 'sed'. Nothing wrong with 'sed' but just a little bit personal feeling. I raise this issue is just fro reminding us about this problem since we both are working on the source code especially for those 'child' class.

robraddi commented 6 years ago

Very true. It can always be done inside vim as well, where it highlights all of the matches. I just wanted to provide a quick and dirty solution. Overall, I agree changing the variables to something less verbose would be nicer.

yunhuige commented 6 years ago

@robraddi Another quick update: I just talked to Vince about our idea using different 'child' class in parent 'Restraint' and 'Sampling' class. And I think we still need to try if we can do it within one child for both parents for each observable. Can you remind me why it's a problem when you tried it last time?

yunhuige commented 6 years ago

@robraddi Well, you told me that you had some issues when you tried to move scripts in both Restraint and PosteriorSampler to the child restraint*.py so we agreed that we may could have separate child for Restraint and PosteriorSampler. I didn't dig into the issue you encountered but I guess it's about the communication between scripts. So there must be a way to fix it(and it shouldn't be difficult).

yunhuige commented 6 years ago

@robraddi It's okay if you already made it work with separate child class. We just need spend a little bit more effort to make each child initialize both restraint and sampling parameters. But still each observable should have their own child.

robraddi commented 6 years ago

@yunhuige , I deleted that last comment I made. I was confused with what you said at first. Yea, this is fine and shouldn't be an issue. My issue was separate from this. (It was in posterior sampler; initialization of the classes were giving me errors. No longer an issue. Switched gears...) I understand what your saying. It should be simple. You're correct.

yunhuige commented 6 years ago

@robraddi You can try to combine them for now and see what problem you will get. We can fix it after together (I will be in the lab next week). You can also work on something else if you want but you don't have to. I think you've done great jobs so far! :)

yunhuige commented 6 years ago

@robraddi Here is one thing we should think about from now on: what function could we merge to get generalized functions in both Restraint and Sampling. One thing I can think of is building/computing reference potentials. Is there anything else?

robraddi commented 6 years ago

SSE?, -logP ?

...side note: "Build Groups Protection factor" in restraint.py simply points to compute see protection factor. (Remove?)

yunhuige commented 6 years ago

@robraddi SSE is a good one but be careful when we are dealing with distances. You can check it in the source code and you will figure out the difference. Basically, sse of noe is related to gamma index and it makes it different from the rest of observables. Don't worry about PF for now since I still didn't get the job done for PF.

yunhuige commented 6 years ago

I changed function names shorter in Restraint (both parent and child) and also Preparation + prep* files.

robraddi commented 6 years ago

@yunhuige, I edited the PosteriorSampler. Exponential distribution is default, and we have almost fixed the issue of only computing the required results.

robraddi commented 6 years ago

@yunhuige I think I just need to make a minor addition to get this to work. I will push in a little bit if I get it working or not. I'll make an issue of what we need to complete if I am unable to get it working.

yunhuige commented 6 years ago

Done! (protection factors --> pf, chemical shifts --> cs, distances --> noe)