Skip to content

Conversation

@NellyMitnik
Copy link
Contributor

No description provided.

calvinp0
calvinp0 previously approved these changes Feb 1, 2023
Copy link
Contributor

@calvinp0 calvinp0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

small change in README file
@alongd
Copy link
Contributor

alongd commented Feb 3, 2023

@NellyMitnik, looks good!
Care to rebase and squash some of the commits?
I would also recommend to remove the white-space from the folder's name

@alongd
Copy link
Contributor

alongd commented Feb 7, 2023

Currently, the background script creates the images folder in the run folder (os.getcwd()), not in the parent folder of the cantera file. Perhaps modify that?

Copy link
Contributor

@alongd alongd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I added some comments

"home = os.getenv(\"HOME\") or os.path.expanduser(\"~\")\n",
"rmg_pypath = os.path.join(home, 'anaconda3', 'envs', 'rmg_env', 'bin', 'python')\n",
"if not os.path.isfile(rmg_pypath):\n",
" raise Error('Could not find RMG-env')\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python does not recognize Error, should be perhaps ValueError

" raise Error('Could not find RMG-env')\n",
"\n",
"######### Edit the path below ##############\n",
"command = [rmg_pypath, '/home/nelly/OneDrive/Linux backup/scripts/Cantera/Flux diagram/background_script.py', species_dict_path]\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this to the paths above to be modified by the user

" \"\"\"\n",
" This func goes over the pdf images in images folder, and converts each pdf image to png image\n",
" \"\"\"\n",
" os.chdir(flux_diagram_folder_path+'/images')\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of flux_diagram_folder_path+'/images', can you do os.path.join(flux_diagram_folder_path, 'images)?

for specie in species_dict.values():
molecule=specie.molecule[0]
smiles=molecule.to_smiles()
if smiles != "[Ne]" and smiles != "[Ar]": #avoid parenthesis for elements - keep convention with Cantera
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[He] will probably need a similar treatment if it's present in a model

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider:

    for smiles in species_output_dict.values():
        if smiles in ['[Ne]', '[He]', '[Ar]']:
            convert_pdf_2_png(smiles[1:3], os.path.join(flux_diagram_folder_path, 'images', f'{smiles[1:3]}.pdf'))
        else:
            convert_pdf_2_png(smiles, os.path.join(flux_diagram_folder_path, 'images', f'{smiles}.pdf'))

" \"\"\"\n",
" This func creates a new folder to each t time interval. Inside that folder, 3 files are created:\n",
" dot, modified_dot, and png file (the flux diagram itself)\n",
" It's importatnt to note that the fluc diagram follows C element --> can be changed\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: flux

" element = 'C'\n",
"\n",
" if t==1:\n",
" t_path=flux_diagram_folder_path+\"/tau\"\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use os.path.join() here as well, and anywhere else when combining paths?

@alongd
Copy link
Contributor

alongd commented Feb 7, 2023

Would you like to mention in the instructions that the users should have dot installed via graphviz (http://www.graphviz.org/download/)? Or is it already specified and I missed it?

"""
The main ARC executable function
"""
args = parse_command_line_arguments()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like args is not being used

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be enough:

    args = parse_command_line_arguments()
    dict_species(args.file)

Copy link
Contributor

@alongd alongd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added two comments

for specie in species_dict.values():
molecule=specie.molecule[0]
smiles=molecule.to_smiles()
if smiles != "[Ne]" and smiles != "[Ar]": #avoid parenthesis for elements - keep convention with Cantera
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider:

    for smiles in species_output_dict.values():
        if smiles in ['[Ne]', '[He]', '[Ar]']:
            convert_pdf_2_png(smiles[1:3], os.path.join(flux_diagram_folder_path, 'images', f'{smiles[1:3]}.pdf'))
        else:
            convert_pdf_2_png(smiles, os.path.join(flux_diagram_folder_path, 'images', f'{smiles}.pdf'))

"""
The main ARC executable function
"""
args = parse_command_line_arguments()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be enough:

    args = parse_command_line_arguments()
    dict_species(args.file)

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.

4 participants