Project

General

Profile

Actions

Bug #1095

closed

"ERROR: undefined outdir" in Frealign Job Uploader

Added by Lauren Fisher over 13 years ago. Updated about 13 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Amber Herold
Category:
Web interface
Start date:
12/16/2010
Due date:
% Done:

0%

Estimated time:
Affected Version:
Appion/Leginon 2.1.0
Show in known bugs:
No
Workaround:

Description

Got this error when trying to upload a frealign reconstruction. Error message comes from inc/processing.inc when $outdir is empty. The problem is there is no $outdir in uploadFrealign.php because it is called $rundir/$jobpath. I think adding a hidden input line setting $outdir to $jobpath will fix this. However, this brings up the problem of appion code not being standardized. Depending on who wrote the code output directories may be called outdir, rundir, path, reconpath, jobpath, etc. And this is just one example of inconsistencies. I know this is not a new issue and the solution is impractical and time consuming, but just wanted to bring it up to see if anyone has any suggestions on how to avoid coding conflicts. Also, as a new appion coder, I don't know which style to follow. Any suggestions?

Actions #1

Updated by Dmitry Lyumkis over 13 years ago

At some point, we may want to go through and standardize inconsistencies in the Appion code, but I think this should be done after the release.

for example:

$outdir vs. $rundir
$sessionid vs. $sessionId vs. $expid vs. $expId

there are other minor ones like:

$apix vs. $pixelsize
$box vs. $boxsize

The problem is that this is a time-consuming fix, because all of the php pages would have to be checked and updated.

Actions #2

Updated by Lauren Fisher over 13 years ago

  • Status changed from Assigned to In Code Review
  • Assignee changed from Lauren Fisher to Dmitry Lyumkis
Actions #3

Updated by Amber Herold over 13 years ago

We could decide on a standard now that we use for new/modified code and refactor the existing code as we can. Anyone else interested in setting a standard? I could start a page to list the inconsistencies that we come across for the next month or so, then we could choose a standard and document it...

Actions #4

Updated by Eric Hou over 13 years ago

Setting standard is not a bad idea, but it still can not prevents people to use any variable name they want.
The best way is to create a base class with most of the default variables (with all the getter and setter).
For any other special variables for different appion processing tools, we really don't need to care about what's the local variable name should be looks like.

Thanks.

Eric

Actions #5

Updated by Amber Herold over 13 years ago

Yes, a base class would be good, but to do that we still need to decide what to include in a base class. I think the first step is deciding on a standard, second documenting it, third writing a base class for future use, then refactoring existing code to use the base class or standard when possible.

This is all part of making appion extensible, so we should be getting to this in the next 6 months anyway.

Actions #6

Updated by Eric Hou over 13 years ago

I am assuming you mean: Planning -> Implementation -> Testing -> Documenting -> Deployment -> Maintenance. Standard Software development process.
We might need to come out a solution to migrate all our appion tools one at the time, so we don't need to do it all at the same time.

Actions #7

Updated by Amber Herold over 13 years ago

No, I'm not talking about sw dev process. When I've worked on projects in the past that needed to do similar large scale refactoring, it has worked well to have a clear document of how to implement the refactoring and ask people to actually do the refactoring when they are editing the file for a bug fix or feature addition since the code will need to be tested anyway. This way the changes are made slowly but eventually the code follows the standard. If we went through and renamed every occurrence of outdir at one time, we would need to test every file that changed. It could be done, but could be error prone or time consuming since some files may be using it for slightly different things.

Actions #8

Updated by Eric Hou over 13 years ago

That’s why I suggest using the base class, and then you don’t need to change the entire Appion tools at the same time (you can change one by one or just the most common one). That way, it also make sure people will follow the standard in the feature because they would have to otherwise it won’t work. Therefore no one needs to read and write any extra document, and no one need to maintain those documentation.
The current Agile methodology in software development is to reduce software documentation as much as possible, because people realize they spend more time to read, write and maintain documentation more than development time.
By the way, I am just making my best suggestion for the team. It does not have to do in my way.

Eric
peace

Actions #9

Updated by Anchi Cheng over 13 years ago

A historical input:

A while ago, we agreed and changed the php code so that:

$outdir is the directory that contains the disk, session, jobtype, but not the runname: For example: /data/appion/10dec16a/extract

$rundir = $outdir + runname, For example: /data/appion/10dec16a/extract/dogrun1

appion python code only accepts $rundir

Actions #10

Updated by Eric Hou over 13 years ago

Great!
I can start to build up these 5 basic variables base on Anchi's input.
I also browsed some of the Appion tools. most of them contain '$description', '$commit', '$command', '$sessiondata', etc...
These can also put in base class.
The nice thing about base class is if the certain tool does not need all the variables, some variable can be empty without any problem.

Eric

Actions #11

Updated by Amber Herold over 13 years ago

Hey guys,
I started a wiki page for all of this info so we can create definitions of the different terminology and keep track of which variations exist and which we want to standardize on:
developers guide

Also, further comments on this topic can be added to #1108

Actions #12

Updated by Lauren Fisher about 13 years ago

  • Assignee changed from Dmitry Lyumkis to Christopher Irving
Actions #13

Updated by Christopher Irving about 13 years ago

  • Status changed from In Code Review to Merge
  • Assignee changed from Christopher Irving to Amber Herold

This has been tested by Lauren and Code reviewed by me. It needs to be merged to the stable branch. It looks like issue #1108 was created for discussion on standardization. This should not be held up any longer.

-Christopher

Actions #14

Updated by Amber Herold about 13 years ago

  • Status changed from Merge to Closed
Actions

Also available in: Atom PDF