harrison-caudill / pylink

Python Link Budget Tools
BSD 3-Clause "New" or "Revised" License
41 stars 10 forks source link

add dvb-s2x and associated tests #2

Closed harrison-caudill closed 7 years ago

harrison-caudill commented 7 years ago

This change is Reviewable

mikesokolsky commented 7 years ago

There's not enough documentation to really follow this right now without reading through the source code itself. The readme refers to the examples, but they are not commented. Is there a place where the classes and their relationships are defined? There's a little documentation in the README about Tributaries, but exactly what they are and how the fit into the larger context of the package is missing for me. Also, docstrings?


Reviewed 13 of 33 files at r1. Review status: 12 of 26 files reviewed at latest revision, 3 unresolved discussions.


README.md, line 1 at r1 (raw file):

Python Link Budget Calculation/Management and General Modelling

Generally this feels more like it's explaining decisions and talking about gotchas rather than introducing what the package is, what it does, and how to use it. What is a tributary, why is this a DAG and how does that work, etc? See the PR-level comment.


README.md, line 6 at r1 (raw file):

This software package is meant to replace the manual-intensive
spreadsheet method.  This package is intended to permit the following
major changes in our methodology:

who's methodology?


README.md, line 33 at r1 (raw file):

 * `python setup.py install -f`
 * `pip install -r requirements.txt`
 * `d=${HOME}/.matplotlib; mkdir -p ${d} ; cd ${d} ; fc-list` matplotlib issue #2919

recognize this is a potential issue, but it doesn't seem appropriate for a general installation guide.


Comments from Reviewable

harrison-caudill commented 7 years ago

Review status: 10 of 30 files reviewed at latest revision, 3 unresolved discussions.


README.md, line 1 at r1 (raw file):

Previously, mikesokolsky (Mike) wrote…
Generally this feels more like it's explaining decisions and talking about gotchas rather than introducing what the package is, what it does, and how to use it. What is a tributary, why is this a DAG and how does that work, etc? See the PR-level comment.

Done.


README.md, line 6 at r1 (raw file):

Previously, mikesokolsky (Mike) wrote…
who's methodology?

Done.


README.md, line 33 at r1 (raw file):

Previously, mikesokolsky (Mike) wrote…
recognize this is a potential issue, but it doesn't seem appropriate for a general installation guide.

I've reworded so it doesn't look like a required step, but rather an FYI.


Comments from Reviewable

harrison-caudill commented 7 years ago

Review status: 10 of 30 files reviewed at latest revision, 3 unresolved discussions.


README.md, line 33 at r1 (raw file):

Previously, harrison-caudill (Harrison Caudill) wrote…
I've reworded so it doesn't look like a required step, but rather an FYI.

Done.


Comments from Reviewable

harrison-caudill commented 7 years ago

@mikesokolsky outta time -- merging.