search

ZhuangLab / storm-control

Microscope control software
Other
65 stars 67 forks source link

Adding Installation examples and files for MERFISH3 Microscope #78

Closed leonardosepulveda closed 6 years ago

leonardosepulveda commented 6 years ago

I added a Hal configuration file for the merfish3 microscope (xml/merfish3_hcam_config.xml), a bat file to launch Hal in the merfish3 microscope using anaconda environments (batFiles/merfish3.bat) and also I added some comments to the installation files. The only change I made (Actually, George Emanuel) was the explicit cast to pointer, that seems to be necessary in Windows 10.

HazenBabcock commented 6 years ago

I appreciate your enthusiasm but it would be better if you could break these requests into smaller pieces that focused on one solving one particular issue. Out of all the changes you want to make I would only accept the ones for the Thorlabs USB camera, the .bat file and the XML config file. Please break these out into two separate pull requests, one for the camera and the other for the two MERFISH3 setup files.

Reviewing by file:

  1. INSTALL.txt - This is incorrect. HAL still works fine without storm-analysis.
  2. INSTALL_MERFISH3.txt - (a) Why this filename? This looks more like instructions for installing using Anaconda. (b) It has redundant instructions about how install storm-analysis, instructions about how to do this are already part of the storm-analysis project. (c) FWIW, the easy way to include MINGW in your path is to just set the PATH variable, there is no need to mess around with the editing SCONSTRUCT file. I'd suggest limiting yourself here to how to install storm-control using Anaconda. Just assume that the user knows how to install git, Anaconda, storm-analysis, etc.. Also rather than adding another INSTALL file I'd just touch up the storm-control/Anaconda instructions that you started on in the original INSTALL file.
  3. cam_offsets_merfish3.txt - This should not be version controlled.
  4. storm_control.pth - Not sure why you feel the need to delete this file. Also it is mentioned in the INSTALL.txt file.
  5. aotf_pass.txt - Really wish you hadn't done this. This information was not ours to share. That was why I did not include it in the project.

Also the easiest way to go forward is probably to start over from the current master, cloned into a new directory, then redo your changes off of that.

leonardosepulveda commented 6 years ago

Hi Hazen,

Thanks for the feedback. I will separate the requests into smaller pieces as you suggest.

Sorry about the aotf_pass, I did not know it could be a problem. I will remove it from my version too.

Thanks a lot!

Leonardo

On Fri, May 11, 2018 at 10:36 PM, Hazen Babcock notifications@github.com wrote:

I appreciate your enthusiasm but it would be better if you could break these requests into smaller pieces that focused on one solving one particular issue. Out of all the changes you to make I would only accept the ones for the Thorlabs USB camera, the .bat file the XML config file. Please break these out into two separate pull requests, one for the camera and the other for the two MERFISH3 setup files.

Reviewing by file:

  1. INSTALL.txt - This is incorrect. HAL still works fine without storm-analysis.
  2. INSTALL_MERFISH3.txt - (a) Why this filename? This looks more like instructions for installing using Anaconda. (b) It has redundant instructions about how install storm-analysis, instructions about how to do this are already part of the storm-analysis project. (c) FWIW, the easy way to include MINGW in your path is to just set the PATH variable, there is no need to mess around with the editing SCONSTRUCT file. I'd suggest limiting yourself here to how to install storm-control using Anaconda. Just assume that the user knows how to install git, Anaconda, storm-analysis, etc.. Also rather than adding another INSTALL file I'd just touch up the storm-control/Anaconda instructions that you started on in the original INSTALL file.
  3. cam_offsets_merfish3.txt - This should not be version controlled.
  4. storm_control.pth - Not sure why you feel the need to delete this file. Also is it mentioned in the INSTALL.txt file.
  5. aotf_pass.txt - Really wish you hadn't done this. This information was not ours to share. That was why I did not include it in the project.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ZhuangLab/storm-control/pull/78#issuecomment-388524165, or mute the thread https://github.com/notifications/unsubscribe-auth/AYzz_lgfF2ebwOk7gGmbWMq_yIv0TWsEks5txkqigaJpZM4T8KSU .

HazenBabcock commented 6 years ago

Just reminding you that I'm still interested in your Thorlabs USB camera fix.