jepegit / cellpy

extract and tweak data from electrochemical tests of cells
MIT License
88 stars 30 forks source link

[WIP] Paper Review #272

Closed TomTranter closed 8 months ago

TomTranter commented 10 months ago

A discussion thread for the review of the Joss paper

TomTranter commented 10 months ago

Can you clarify why the order of the authors was chosen to be the way it is please.

TomTranter commented 10 months ago

I have installed with conda and following the instructions here https://cellpy.readthedocs.io/en/latest/examples_and_tutorials/basics.html#the-getting-started-with-cellpy-tutorial-opinionated-version I have copied tried loading the example data files you provide in the testdata folder but getting this error NoSuchModuleError: Can't load plugin: sqlalchemy.dialects:access.pyodbc

import cellpy
import os
data_files = os.listdir("cellpyfiles")
for i, name in enumerate(data_files):
    print(i, name)
data_filepath = os.path.join("cellpyfiles", data_files[0])
c = cellpy.get(data_filepath)
TomTranter commented 10 months ago

Seems like sqlalchemy-access wasn't installed

jepegit commented 10 months ago

Can you clarify why the order of the authors was chosen to be the way it is please.

Hi @TomTranter. Julia (@juliawind ) is the one that has pushed for this paper and done most of the writing. So I think she deserves to be first author. Asbjørn (@asbjorul) and Muhammad (@Ozzstein ) contributed less to the paper. But they have contributed more on the actual code. Then its me as the last author (the "creator" of cellpy). Last author appears to be "a thing" in several scientific fields, especially in material science / battery research. Good for me :-)

jepegit commented 10 months ago

Seems like sqlalchemy-access wasn't installed

Hi @TomTranter. Yes, sorry about that. sqlalchemy-access didn't have a conda package (at least last I check / or I could not find it). If I have understood condacorrectly, all dependencies needs to be available as its own condapackage. I thought that I should create a condapackage for it myself, and then everything would be smooth. But have I done that? Nope.

TomTranter commented 10 months ago

Seems like sqlalchemy-access wasn't installed

Hi @TomTranter. Yes, sorry about that. sqlalchemy-access didn't have a conda package (at least last I check / or I could not find it). If I have understood condacorrectly, all dependencies needs to be available as its own condapackage. I thought that I should create a condapackage for it myself, and then everything would be smooth. But have I done that? Nope.

Ok - seems like it should be possible and I see you have the environment.yaml file with the pip section including the package but it was not referred to in the installation docs and took some time to figure out

TomTranter commented 9 months ago

@jepegit Any further comments? - I have paused my review awaiting your response

jepegit commented 9 months ago

Hi @TomTranter. Seems we need a bit more time than I thought. Think I have succeeded in making a conda package for sqlalchemy-access, at least. Probably need at least one more week to improve the documentation and tweak the conda recipe for cellpy. Sorry for that.

jepegit commented 8 months ago

Summary of tasks:

jepegit commented 8 months ago

conda package for sqlalchemy-access made and order of authors clarified.

TomTranter commented 8 months ago

Re-installed following updates and ran examples. Works for me on windows. In general I find the package a little non-intuitive to use but the classes and functions are documented well enough to figure it out. My review is finished. Thanks to the authors for making this useful package open source