Modernize #18

2021-07-11T11:04:59Z

mih commented

2021-07-11 11:04:59 +00:00

(Migrated from github.com)

Just just be a make still, but:

ensure virtual env with proper dependencies and remodnav version
all imports on top (immediate and complete dependency check)
no longer piping outputs (did prevent progress bars from showing)
avoid computing some pieces twice
consolidate datalad API usage on a single function (get)
some more explicit function names
some import duplication removed

Just just be a `make` still, but: - ensure virtual env with proper dependencies and remodnav version - all imports on top (immediate and complete dependency check) - no longer piping outputs (did prevent progress bars from showing) - avoid computing some pieces twice - consolidate datalad API usage on a single function (`get`) - some more explicit function names - some import duplication removed

adswa (Migrated from github.com) approved these changes

2021-07-11 13:13:17 +00:00

adswa (Migrated from github.com) left a comment

This looks good, and makes a much nicer terminal output! I added a suggestion to get rid of one remaining warning. We should, however, also update the README accordingly. Currently, it still advises to install the requirements by hand.

code/mk_figuresnstats.py

					
				@ -861,14 +865,14 @@ def savegaze():

				            sampling_rate=1000.0,

            vel_lim=(0.001, 1000))

While we are at it, we could set the lower bounds of the y-axis to not exactly zero. The data is logscaled, and this results in

/home/adina/repos/papers/paper-remodnav/remodnav/remodnav/tests/utils.py:124: UserWarning: Attempted to set non-positive bottom ylim on a log-scaled axis.
Invalid limit will be ignored.
  ax2.set_ylim(vel_lim)

```suggestion vel_lim=(0.001, 1000)) ``` While we are at it, we could set the lower bounds of the y-axis to not exactly zero. The data is logscaled, and this results in ``` /home/adina/repos/papers/paper-remodnav/remodnav/remodnav/tests/utils.py:124: UserWarning: Attempted to set non-positive bottom ylim on a log-scaled axis. Invalid limit will be ignored. ax2.set_ylim(vel_lim) ```

adswa commented

2021-07-11 13:21:08 +00:00

(Migrated from github.com)

EDIT: Disregard this. ~~Oh one more thought: remodnav doesn't have datalad as a requirement, as far as I remember. Can we add this, or point people towards installing it themselves?~~

EDIT: Disregard this. ~Oh one more thought: ``remodnav`` doesn't have datalad as a requirement, as far as I remember. Can we add this, or point people towards installing it themselves?~

adswa commented

2021-07-11 13:49:10 +00:00

(Migrated from github.com)

I pushed a README update

mih commented

2021-07-11 15:39:25 +00:00

(Migrated from github.com)

Thx!

Sign in to join this conversation.

No reviewers

adswa