search

b-pardi / BraTaDio

Python based computational for for analyzing quartz crystal microbalance with dissipation (QCM-D) experimental data
MIT License
0 stars 0 forks source link

Improve README and install instructions #2

Closed kstenio closed 3 months ago

kstenio commented 3 months ago

Hi @b-pardi, sorry for taking this much time for a 1st round review. It took a lot of time for another reviewer to appear, and now I'm a little busier than before. Either way, here we are!

As another reviewer did with me while I was in your shoes, I am opting for opening multiple issues. So you can work on each one separately. This one is mostly regarding install instructions.

Your readme is fine, but could benefit from a finer structure. You can make a little introduction on what the software does and limitations. Something similar to the paper abstract.

Then, you should focus on a mini-tutorial on installing the application. The install_packages script seems totally unnecessary since I did not use it. I would recommend some code instructions or an ordered list of topics.

What I did:

# Cloned the repository
git clone https://github.com/b-pardi/BraTaDio/
# Entered the app folder
cd BraTaDio
# Created an Python virtual environment
pyton -m venv .venv
# Sourced the venv
source .venv/bin/activate
# Installed packages from requirements.txt
pip install -r requirements.txt
# Run the application
python main.py

Also worth mention that I am under Linux (Ubuntu 22.04 to be precise), and had no problems in setting up the environment, but I experienced a warning when generating graphics: findfont: Font family 'Arial' not found. This is a little obvious, as I do not have the Arial font on my system. I would recommend rely on font family other than fontname for Matplotlib.

Finally, after the install instructions, you can resume your readme as it is now.

b-pardi commented 3 months ago

Thank you @kstenio for getting to review it! No worries about taking some time to get to it, I appreciate it regardless. I have added a few more details like specifying how to setup a venv, but I also have responded to much of the feedback of the other reviewer as well, so let me know if the changes are satisfactory. I'll close the issue for now, but please let me know if there's more I need to do! Also I do appreciate the multiple separate issues, it makes these much more manageable.

kstenio commented 3 months ago

I've checked the changes, and for me they are sufficient :-)

b-pardi commented 3 months ago

@kstenio I closed this issue before I saw your last comment on the font issue. Can you tell me what line it was you got the issue? Cuz I believe I used fontfamily everywhere.

kstenio commented 3 months ago

It is a warning that appears when saving a graphic.

image WARNING matplotlib.font_manager:font_manager.py:1371 findfont: Font family 'Arial' not found.

kstenio commented 3 months ago

Hello @b-pardi, any news on the above warning? I tried replacing "Arial" and "font" for "sans-serif" and "font-family" in some parts of the code (json files and analyze.py), but another issue happens. It would be nice fixing this, but since it is just a warning, I think it is OK. (but you should consider opening an issue later...)

b-pardi commented 3 months ago

@kstenio I went ahead and opened a new issue #8