-
Notifications
You must be signed in to change notification settings - Fork 9
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
Improved API and Developer guide #220
Conversation
Restructured API and dev guide, also added lots of content to dev guide. For now it looks good. Definitely still lots to add, but it's a very nice change compared to what it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Wow, that is a lot. I'm honestly worried it's a little bit too much detail (and possibly duplication). While it might be nice for there to be a lot of information, I'm worried about long term maintainability if things are mentioned in more than one place. From reading the documentation, it looks like the documentation is ordered by module/package. I think it might be an idea to order it more by purpose, by which I mean for example "I want to create a new distribution", "I want to create a new privacy package implementation", "I want to understand how the different classes interact". I'm curious what your thoughts about this!
Co-authored-by: qubixes <44498096+qubixes@users.noreply.github.com>
The dev overview and distribution pages now contain more essential and condensed information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting somewhere! I'm sorry there's another bunch of comments from my side, but that happens sometimes with these big changes!
|
||
For a detailed overview of classes, methods and attributes mentioned on this page, refer to the :doc:`/api/metasyn`. Clicking on object names will automatically take you to their API reference page. | ||
|
||
Distribution subpackage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this better suited for the automated API Reference? We can also write more documentation strings for the modules/packages themselves, if there is not enough context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like having it here. I think this page serves to provide a birds-eye overview of what's relevant to creating new distributions, it doesn't bloat the page up, and provides nice references to the API for who-ever wants to read more. Once the API is more fleshed out, maybe information can be moved over there, but until then I'd rather keep this.
|
||
Finally it contains the :func:`~metasyn.distribution.metadist` decorator, which is used to set the attributes of a distribution. | ||
|
||
BaseDistribution class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would turn it around between the class and the decorator. I would start with the decorator, since it is the one developers work with the most. Then we can explain that the decorator sets these attributes for the developer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, keeping redundancy and ease of maintaining in mind, I am tweaking the page a bit.
|
||
The :obj:`~metasyn.MetaVar` class contains functionality for: | ||
|
||
- **Detecting variable types**: The :meth:`~metasyn.MetaVar.detect` method detects the variable class(es) of a series or dataframe. This method does not fit any distribution, but it does infer the correct types for the :obj:`~metasyn.MetaVar` and saves the ``Series`` for later fitting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for most cases, the MetaVar is more of a passthrough class. (I haven't worked on it really for a really long time!), so I think we don't need to have this part about the methods here (it can stay in the API ref). One thing that may be nice is a small section on how to create a MetaFrame without data, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rephrased it a bit. I do like the information being here for now - as it helps provide a fuller context.
- **Converting to and from a dictionary**: The :meth:`~metasyn.MetaVar.to_dict` method creates a dictionary from the variable. The :meth:`~metasyn.MetaVar.from_dict` method restores a variable from a dictionary. | ||
|
||
|
||
Subpackages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can all go into the API as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have significantly shortened the subpackages and submodules sections. I like to keep them like this for now, and then soon once the API has been fully fleshed out, see how they can be condensed further.
Co-authored-by: qubixes <44498096+qubixes@users.noreply.github.com>
210bf30
to
753e355
Compare
I have further condensed and changed information based on comments. There's still some (technically) redundant information, but for now, it helps paint a clear picture for developers wanting to get started. I aim to improve the docstrings + API next; once that has been done, I will reevaluate the content on the dev guide pages and further condense/remove it where necessary. |
753e355
to
3cf84a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the work! Let's merge it soon!
Co-authored-by: qubixes <44498096+qubixes@users.noreply.github.com>
Developer guide is now a full section, as opposed to the tiny (and very outdated) page it used to be.
API reference has also been restructured to be easier to navigate.
Lots of changes and information added - so please give it a thorough review!