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

made stats functions consistent with BaseParser #42

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

DHekstra
Copy link
Contributor

@DHekstra DHekstra commented Jun 28, 2023

In this PR,

  1. Made all four rs.cc functions compatible with the BaseParser (see also https://github.com/rs-station/careless/blob/main/careless/stats/parser.py), itself taken from careless.stats without modification. This provides the user with a uniform interface and makes it easier to integrate the functions in code that just saves the results as CSV files and figure files by virtue of the -i, -o, and '-s` flags introduced by the BaseParser. I did not modify any calculations.

  2. Changed datatypes from MTZInt to int32 in the results DataFrame as introduced by Kevin in the careless.stats functions. This locally addresses the issue seaborn 0.12.x breaks plotting functionality #36

  3. In sns.lineplot calls I replaced the ci keyword with errorbar

  4. Made the analyze_...() functions compatible with both MTZ path and rs.DataSet input to facilitate use as function calls within notebooks. I do so using the following:

    if type(mtzpath) is rs.dataset.DataSet:
        mtz=mtzpath
    else:
        mtz = rs.read_mtz(mtzpath)
  1. I have tested each function on Careless output.

@DHekstra
Copy link
Contributor Author

OK, let me look into the "Test Documentation" error first

@DHekstra
Copy link
Contributor Author

For my records: I have tested these changes in /n/holyscratch01/hekstra_lab/dhekstra/careless-examples/e35 (rs.cchalf, rs.ccpred, rs.ccsym) and tested rs.ccanom on ~/HEWL/hewl_xval_0.mtz.

@dennisbrookner
Copy link
Member

dennisbrookner commented Jun 30, 2023

The failing test build of the docs is related to this:
sphinx-contrib/autoprogram#55

Briefly, in order for the docs to build properly, argparse usage must look like this:

def parse_arguments():
    parser = argparse.ArgumentParser()
    # blah blah blah
    return parser

def main():
    parser = parse_arguments()
    args = parser.parse_args()

and NOT like this:

def parse_arguments():
    parser = argparse.ArgumentParser()
    # blah blah blah
    return parser.parse_args()

def main():
    args = parse_arguments()

nor any other equally valid paradigm

@dennisbrookner
Copy link
Member

To further clarify the situation with the docs - this is the relevant code from https://github.com/rs-station/rs-booster/blob/main/docs/CC_plots.rst that auto-builds stuff:

.. autoprogram:: rsbooster.stats.ccsym:parse_arguments()
   :prog: rs.ccsym

.. autoprogram:: rsbooster.stats.ccanom:parse_arguments()
   :prog: rs.ccanom

.. autoprogram:: rsbooster.stats.cchalf:parse_arguments()
   :prog: rs.cchalf

.. autoprogram:: rsbooster.stats.ccpred:parse_arguments()
   :prog: rs.ccpred

So as written, autoprogram is expecting each of these files to contain a function parse_arguments() that returns some sort of ArgumentParser object.

I think that this structure is compatible with the BaseArguments system that is being adopted in this PR. Let me know if that isn't the case, and we can reconsider the best options.

Copy link
Member

@dennisbrookner dennisbrookner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me!

@DHekstra DHekstra merged commit 162a4ce into rs-station:main Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants