Seqfold - Githubissues

folago commented 1 year ago

Add seqfold package, Go port of the python project at: https://github.com/Lattice-Automation/seqfold

folago commented 1 year ago

Done.

What about README and LICENCE? Keep? Remove? Change?

folago commented 1 year ago

Actually since the cmd is not there anymore the README can be chopped.

folago commented 1 year ago

Ah also I made fixups, I usually push more fixups until it's OK an then rebase and force push the branch. I am not sure how you prefer things here, also I can split the PR in more commits if you like. Let me know.

folago commented 1 year ago

Just realized what you meant by part of the checks package 😂 Also added tests.

TimothyStiles commented 1 year ago

@folago go linter is picking up some redundant code and other things.

https://github.com/TimothyStiles/poly/actions/runs/4441863344/jobs/7863721360?pr=295#step:3:71

folago commented 1 year ago

OK I did a quick pass, I'll have more time later this week.

folago commented 1 year ago

@folago go linter is picking up some redundant code and other things.

https://github.com/TimothyStiles/poly/actions/runs/4441863344/jobs/7863721360?pr=295#step:3:71

Fixed most of it, the redundant code looks like a false positive:

$ golangci-lint run ./...
dna.go:478: 478-509 lines are duplicate of `dna.go:569-600` (dupl)
var DNA_INTERNAL_LOOPS = LoopEnergy{
    1:  {0, 0},
    2:  {0, 0},
    3:  {0, -10.3},
    4:  {0, -11.6},
    5:  {0, -12.9},
    6:  {0, -14.2},
    7:  {0, -14.8},
    8:  {0, -15.5},
    9:  {0, -15.8},
    10: {0, -15.8},
    11: {0, -16.1},
    12: {0, -16.8},
    13: {0, -16.4},
    14: {0, -17.4},
    15: {0, -17.7},
    16: {0, -18.1},
    17: {0, -18.4},
    18: {0, -18.7},
    19: {0, -18.7},
    20: {0, -19.0},
    21: {0, -19.0},
    22: {0, -19.3},
    23: {0, -19.7},
    24: {0, -20.0},
    25: {0, -20.3},
    26: {0, -20.3},
    27: {0, -20.6},
    28: {0, -21.0},
    29: {0, -21.0},
    30: {0, -21.3},
}
dna.go:569: 569-600 lines are duplicate of `dna.go:478-509` (dupl)
var DNA_HAIRPIN_LOOPS = LoopEnergy{
    1:  {0, 0.0},
    2:  {0, 0.0},
    3:  {0, -11.3},
    4:  {0, -11.3},
    5:  {0, -10.6},
    6:  {0, -12.9},
    7:  {0, -13.5},
    8:  {0, -13.9},
    9:  {0, -14.5},
    10: {0, -14.8},
    11: {0, -15.5},
    12: {0, -16.1},
    13: {0, -16.1},
    14: {0, -16.4},
    15: {0, -16.8},
    16: {0, -17.1},
    17: {0, -17.4},
    18: {0, -17.7},
    19: {0, -18.1},
    20: {0, -18.4},
    21: {0, -18.7},
    22: {0, -18.7},
    23: {0, -19.0},
    24: {0, -19.3},
    25: {0, -19.7},
    26: {0, -19.7},
    27: {0, -19.7},
    28: {0, -20.0},
    29: {0, -20.0},
    30: {0, -20.3},
}

The third element is already different 🤷🏻

folago commented 1 year ago

I'd say it's time for a rebase? And maybe I can split the PR in more commits with a decent commit message.

The only missing part that I can see (a part from some more docs and comments) is the complements since transform.ComplementBase only seems to complement DNA

folago commented 1 year ago

Sorry for the wild rebases but it was getting a too messy for my taste, I have a local copy of the last version in case you want to keep a track of things (especially your commit @TimothyStiles).

Some changes in no particular order:

no more panics, just passing errors as it is supposed to be
small docs improvement
fix linter false positive

This https://github.com/TimothyStiles/poly/pull/295#issuecomment-1490725652 is IMHO still open, should I just use transform.ComplementBase for the DNA? Should I add transform.ComplementBaseRNA?

TimothyStiles commented 1 year ago

Sorry for the wild rebases but it was getting a too messy for my taste, I have a local copy of the last version in case you want to keep a track of things (especially your commit @TimothyStiles).

Some changes in no particular order:
* no more panics, just passing errors as it is supposed to be

* small docs improvement

* fix linter false positive
This #295 (comment) is IMHO still open, should I just use transform.ComplementBase for the DNA? Should I add transform.ComplementBaseRNA?

No problem on the rebases. Using transform.ComplementBase for DNA and adding transform.ComplementBaseRNA would be excellent @folago!

folago commented 1 year ago

No problem on the rebases. Using transform.ComplementBase for DNA and adding transform.ComplementBaseRNA would be excellent @folago!

I was looking at the transform package again and found a couple thing I think are worth mentioning:

ReverseComplement and Complement have different behavior than ComplementBase since the former two do not use the latter but access the complement table directly. So a missing value in the first two function results in a 0 while ComplementBase gives a space .
Test should be, IMHO, repeatable and predicable random test are neither.
Shouldn't we just use a strings.Builder instead of punching a hole trough the abstraction offered by the standard library and avoid unsafe?
Since you added the align package I'd add the gap - to the table.

Apologies for being pedantic, I am up for submitting changes to what I mentioned just wanted to have the green light.

I also can just copy/paste the code as is to have a reverse for RNA and we can talk about this another time (or never if this is good enough™)

folago commented 1 year ago

As for the types and functions exported it depends on the use of this package, if it is only going to be used to Fold() and print the results we can create result type and just export Fold() and Result, for example:

type Reusult struct {
    result []nucleicAcidStructure
}
func (r Reusult)DotBracket(){...}
func (r Reusult)MinimumFreeEnergy(){...}
func (r Reusult)PrintTabularResult(){...}
func (r Reusult)Whatnot(){...}

folago commented 1 year ago

I pushed few more style changes. I have a question on the packages layout, if eventually linearfold is merged will the fold package undergo a breaking change?

TimothyStiles commented 1 year ago

I pushed few more style changes. I have a question on the packages layout, if eventually linearfold is merged will the fold package undergo a breaking change?

that's actually something I've been wondering too. Thoughts were.

Rename the Fold function to Zuker so that it would be called fold.Zuker and we could name the linear fold function as fold.Linear.

Think that would work @folago?

folago commented 1 year ago

Yeah that should work 👍🏻

TimothyStiles commented 1 year ago

@folago MERGED.

Could use a little more test coverage but this is great and I'm trying to get all outstanding PRs merged before I move poly to a new org.

jjti commented 1 year ago

This is sick, @folago. I didn't review in detail, but I love it. Would be curious about the speed up of this vs the python version

folago commented 1 year ago

This is sick, @folago. I didn't review in detail, but I love it. Would be curious about the speed up of this vs the python version

Thanks! I did not do a thorough comparison, just a quick test using cython to run seqfold and the speed seemed comparable.

I did not look at any profiles though, so maybe there are some easy wins.

bebop / poly

Seqfold #295