could you please move this into a separate branch.

we simplified this now to

lines = fh.readlines()

in master an just always assume that fh is a filehandle.

this means here you need to reset the filehandle with fh.seek(0) between calls.

duplicate

a better name is mdengine

gromacs module seems to be wrong.

Does this work with jinja2? In the draco example you used == for comparison.

yes, this works too.. I tried it

ah yes.. nevermind, I forgot to change that

Lets maybe stick to one, just in case users get confused when comparing different templates :)

yes, this was faulty syntax anyway

You can probably get rid of the print statement here?

yeah this is an old test..

This would generate an ugly stack trace for the CLI user. In regards to #38 we should use click.echo and throw some error at the user and then exit via sys.exit(1).

Maybe we can add our own warning and critical wrappers.

def warning(msg):
    click.echo("WARNING: " + msg, ...)

def critical(msg):
    click.echo("CRITICAL ERROR: " msg, ...)
    sys.exit(1)

That would make consistent formatting easier.

@kain88-de, @mimischi: I think this would be a good way to stay consistent!

@MSiggel Should we create a new PR for this and merge it into master, so you can adapt it in here?

Same as above.

Maybe we could also combine detect_md_engine and formatted_md_engine_name into one function, as the code is pretty much duplicated.

We could do something along the lines of:

def get_md_engine(modulename, return_module=True):
    obj = None
    mdengines = ['gromacs', 'namd']

    # Loop over all known mdengines
    for engine in mdengines:
        # The name is inside the given module name
        if engine in modulename:
            # Prepare to return the module as string
            obj = engine

            # We want to return the actual module
            if return_module == 'module':
                obj = eval('mdengine.{}'.format(engine))
            break

   # If we have not found the modulename after the for-loop, we exit
    if not obj:
        click.echo('ERROR We were not able to detect any module name. Did you specify that?')
        sys.exit(1)

    # Return the object, because we found it
    return obj

To complicated.

In gromacs.py / namd.py

import ...

NAME = "GROMACS"

def ...

Any function that later wants to name can do.

get_md_engine(module).NAME

The idea is that every module carries a formatted name. This means we don't need to distinguish between the case where we want a name or a module.

formatted_md_engine_name(modulename)

this is not called anymore so it can be removed. as soon as the module was detected the respective engine files "know" their formatted and they can be hard coded.

I don't think allowing multiple=True makes sense here. Bu default we analyze all module anyway for the csv output, and plotting will currently fail.

long option names have two leading dashes --

I think this should be removed again. But in general, don't add too many short options. Only for options that are set often and that can be set to have the same letter as the start of the long version.

Maybe rather add this into the generate function of the NAMD module.

if one moves it to the namd generate function it will be shown x times for each folder which is generated..maybe a new util fxn? we'll probably need such a warning for other engines too.

Will we keep this message when merging? We should not merge anything that is not fully functional? If we are sure that it is working, which the unit/integration test should pledge for, then we can get rid of that message entirely. Also trying it out on the clusters would help reassure that everything is working 🔥

Do you specifically catch this exception somewhere? If not I would remove the import.

I talked with @mimischi today. This would be better suited in the mdengine/init.py file.

Please write in the error message that the mdengine couldn't be detected from the module and what mdengines are known to mdbenchmark.

This comment still says GROMACS.

We probably do not need this here. We added it to the GROMACS module, because we added more categories to your Sim objects at some point. All new Sim objects for NAMD will include that category time.

Same thing here. I do not think we will need this check. Just using sim.categories['module'] for NAMD should be fine. What are your thoughts @kain88-de?

yeah this is only needed for gromacs

What about this TODO?

for now i would leave this. Once namd is merged we can think about how we manage this for all engines i guess?

Comment says GROMACS.

Do we want to keep it this way? I initially thought we will use the host, module and gpu values for the output file name? This way we could prepare for #24, where we would specify different columns to group by. These be used for the filenames..? Or am I overthinking this?

you are overthinking this.

This is probably obsolete

Do we need default='' here? We can probably get rid of it

yeah, this shouldn't have a default.

This comment throws an pep8 error

Will we keep this message when merging? We should not merge anything that is not fully functional? If we are sure that it is working, which the unit/integration test should pledge for, then we can get rid of that message entirely. Also trying it out on the clusters would help reassure that everything is working 🔥

This is very minor, but the syntax for the namd2 apoa1 call is not the one that we put into templates/draco. Maybe we should put them to the same state :)

We probably can skip the first line (ATTENTION is not needed when the message is prefixed with a bold and colored WARNING). Use something like this instead: console.warn('NAMD support is experimental. All input files must be in the current directory. Parameter paths must be absolute. Only crude file checks are performed!')

@MSiggel I could not find your comment anymore. I just simplified your previous code, which repeated the console.error lines for all three cases.

If you are saying that we need to change the structure in general, then we should do this. Your previous code did not do it either.

@mimischi: no it's fine! I misunderstood your code at first, but it's much better the way it is now! I also agree with your other changes concerning the order of error message etc. I just quickly threw it together and didn't think about it too much..

Sure you want to be OK with bench.log being deleted?

We would only call the cleanup when restarting, so it would be best to get rid of the log for the previous run.

oh sorry I meant bench.job the submit script.

Yeah, I only put it there so the test would pass. Could have used @pytest.mark.skip for that.

this doesn't work because mdsynthesis prints the whole path of a leaf. I thought it would only give the filename. This means you need to update the regex of bench.job to allow for an arbitrary beginning of the string.

isn't there a click option to print this automatically?

Only if we define it as an argument, then we could use click.File inside of that definition.