Skip to content

Conversation

@barlehmann
Copy link

This is a pull request by Bar Lehmann

@leej3
Copy link

leej3 commented Mar 19, 2020

Looks good. My pointers as you embark on this is:

  1. Keep the goal in mind. That goal will be to tick the boxes of the rubric, do a fun/useful project, or a bit of both. The scope of the project is broad and may take a lot of code to implement. Don't get lost in the details of the code. Most of your effort from the perspective of grading is to create a repo of code that is well documented, well organized, and well tested. From the perspective of a scientist most of your effort should be spent learning how tools like MNE think about and implement the functionality that you want. That means many hours of learning from their documentation rather than writing your own code. Although as the saying goes... a week of coding can save you an hour of thought.

  2. Having a clear answer to point one will help you pitch the project. For example, you could acknowledge that you are duplicating (or wrapping) functionality of MNE for the purposes of learning. That is a lot safer than attempting to make a genuine contribution by creating a new tool. Generally the answer to such an endevour is "but have you seen the pre-existing tool X?". The most effective use of your time is to contribute to open science community-driven efforts in your field rather than working on your own to tool, putting it on GitHub, and calling it open.

  3. Dropping the scope of the project might help you. Preprocessing and visualization of this data require large amounts of expertise. You don't necessarily want to expose a user of your package (read grader of your project) to that expertise. If you are putting a lot of time into this and want to achieve all of these things you a more than welcome to and we can provide help as you encounter the various issues you run into. But at the very least define clear stepping stones along your path to glory so that if it all goes pear-shaped you have a clear achievement and a gradable project rather than "hmmm, it didn't quite come together". You show evidence of this, I just want to emphasize it.

barlehmann and others added 25 commits March 19, 2020 20:08
Commiting first copy of pycharm (.py) python code file that I have been working on during the past month to read EEG files and begin preliminary exploration.
…ng with date and time information, to compare with values from biometric insturments such as EEG and GSR. Also some initial coding in the main python file for reading and visualizing this information.
…pearson r for variables that would like to explore in Mood_Focus_Table from Excel
…me feedback on related to how to graph the input from a specific sensor. I am also adding the EEG file that is being analysed
…ich can do pearson correlation analysis on two columns of data such as mood and alpha amplitude, and also adding the append_table_function which can append this same overarching spreadsheet of subjective and biometric data from new records, updating it with new relevant entries/recordings.
… more concise and clear comments to guide reader through each function
q
q
Merge branch 'master' of https://github.com/barlehmann/EEG_GSR_Wellness_Tracker

i
'bad files'

q
@barlehmann
Copy link
Author

Dr. Lee, thank you for your response, this was certainly helpful to get my tests to run. However, after merging your changes, I began getting the following (below) error message after running pytest, suggesting that pytest is not connecting the graph module with the test_graph in the tests folder. I will continue to research this problem but wanted to keep you posted.

ImportError while importing test module '/Users/barlehmann/Desktop/Python_project/github_project/EEG_GSR_Wellness_Tracker/tests/test_graph.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/test_graph.py:2: in
import graph
E ModuleNotFoundError: No module named 'graph'

@leej3
Copy link

leej3 commented Apr 30, 2020

correct. The correct pattern when importing from the installed package is:

from eeg_gsr_wellness_tracker import graph

The way you were importing only works if you are importing files from the directory your code is running in (not convenient/robust).

@leej3
Copy link

leej3 commented Apr 30, 2020

Apologies. There was a init.py missing from that pull request. You need to include that in the directory eeg_gsr_wellness_tracker

@barlehmann
Copy link
Author

Dr. Lee, thanks for the information and suggestions. My tests presently both have the format you reccomended ( i.e from eeg_gsr_wellness_tracker import graph ) and I also added and empty init file to the directory you specified, however I am still getting the same error message.

barlehmann added 3 commits May 2, 2020 17:47
 Please enter the commit message for your changes. Lines starting
d
anch master
@barlehmann
Copy link
Author

image

@barlehmann
Copy link
Author

the tail end of my last message was cut off:

image

@leej3
Copy link

leej3 commented May 4, 2020

Thanks for the feedback. You are going about these problems in the correct way. I wouldn't be worried about what you have to show at the moment. You have demonstrated hard work and have met most of the rubric. Engaging with testing and working out some of the sordid details (which we can of course help you with) is an immensely valuable lesson to work through.

Some details to help you pass your tests both locally and on circleci:

you need to include pyopenxl and mne as dependencies for the project (install_requires argument to setup or use a requirements.txt file).

When you install the code it gets relocated into the site-packages directory of your current python interpreter. This means that relative paths to data will both fail. Data should not be stored in the code directory and should never be referenced using an absolute path (they will fail for everyone but you on your current computer). If you do need data it for tests (which you likely will, and in this case do), put it in the tests directory. That way, the relative path makes sense in the context of the location of the test file.

If testing the equality of two arrays the best thing to use is probably numpy.allclose or a similar function. Perhaps I mislead you with that template code I gave you. I presented that to indicate the sorts of things you should be testing (input and output of functions rather than their internals).

@barlehmann
Copy link
Author

image

barlehmann and others added 4 commits May 4, 2020 20:35
fix paths to sample data used for tests

remove some superfluous code

add test to check that when sample data is read the first
three values are as expected
data should not be hardcoded or stored in the code directory. These should be
generic functions that operate on paths defined by the user of the code

superfluous data is removed

global variable was removed. a good heuristic is that if you are using a
global variable you are doing something wrong

removed additional keyword arguments to the io function from mne. You only
specify keyword arguments if you want to. You don't have to specify a value
for them if you do not want.

do not use "pass". Not sure why it was included here
@leej3
Copy link

leej3 commented May 5, 2020

ok I have created a new pull request with a demonstration of a test that executes one of your functions and checks that the result is as expected.

I have modified the function in the process. Your function was setting a global variable. It shouldn't do that. And append_table.py should not need data. That should be provided by the user.

Please read through the code/comments/and commit messages and try to understand the logic of both the test and the function it tests as I have implemented it.

Have a look at this example as well as the commit messages
@barlehmann
Copy link
Author

Thank so much, that certainly helps me understand some of the issues I was having with there needing to be a data directory separate from the others, and that the global variables were causing some issues. I will certainly be looking into the logic of the test functions as you have implemented them. Much appreciated.

@leej3
Copy link

leej3 commented May 5, 2020 via email

@leej3
Copy link

leej3 commented May 9, 2020

Impressive work. You really threw yourself at this. Well done. You have used of a wide range of advanced packages to achieve your goals and written it in a modular way that will make parts of it reusable for future sustainable expansion.

For future improvement, look at some of the other projects for examples of testing. For example, this example of using pandas tools for testing when using data frames will be helpful to the sort of work you are doing.

Sometimes the python style you use is odd, although often harmless from the perspective of execution of the program. I'm not sure where you picked them up. For example the syntax

print\
("hello")

Or using multiline strings in the code as a mechanism to comment. Typically text editors will nicely wrap comments and prepend them with "#" so I just use that. Overall just keep an eye on pep8 as your python skills develop.

Variable scoping was one issue that I think you should do a little reading about. Justified usage of global variables is exceptionally rare. You should typically only be using variables that you have imported from somewhere, defined in the scope of the current module, have recieved as input to a function, or defined within the current function's scope. Getting this right early on is important, otherwise you will adapt your coding in abnormal ways (like global variable usage) and suffer as a programmer in the long run.

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