Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data Model & OME-TIFF Docs rework #434

Merged
merged 17 commits into from Aug 15, 2013
Merged

Data Model & OME-TIFF Docs rework #434

merged 17 commits into from Aug 15, 2013

Conversation

hflynn
Copy link
Member

@hflynn hflynn commented Aug 7, 2013

This PR addresses tickets 10695, 11278 and 11279 which are all linked under https://trac.openmicroscopy.org.uk/ome/ticket/10274

It aims to

  • clarify and improve the main index page so people can find what info we have more quickly
  • restructure the docs into clearer categories
  • make a more approachable OME-TIFF landing page (ome-tiff/index)
  • update the list of OME-TIFF supporters and make it easily accessible
  • add the XML validation method (currently explained in FAQs only) and temporarily remove reference to the online validator which is not currently working

OMEXMLService service = factory.getInstance(OMEXMLService.class);


.. seealso:: :bf_doc:`developers/export2.html` and :bf_doc:`developers/conversion.html`
Copy link
Member

Choose a reason for hiding this comment

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

These 2 hyperlinks could use a more explicit test

@hflynn
Copy link
Member Author

hflynn commented Aug 7, 2013

Bah, when they didn't build correctly locally I thought it was something to do with the conf.py being at the top level & forgot to check they looked okay on the staging version - fixed now anyway.

--------------------

In some cases, it is useful to extract specific parameters or tweak
certain values in a dataset's OME-XML metadata block. Further guidance on :doc:`using-ome-xml` is available, but below is a brief example of the
Copy link
Member

Choose a reason for hiding this comment

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

need for a line break here

@sbesson
Copy link
Member

sbesson commented Aug 8, 2013

Overall, the documentation looks much clearer in my opinion. Below are a couple of minor comments.

The ome-tiff/tools page certainly lacks a bit of consistency:

  • some command are not using any markup, some other are using simple quotes (e.g. xmlvalid)
  • some code blocks have no separator, other have a path followed by a dollar sign. We should at least be consistent per page.

There is a also commented block about the online validator in the same page. Is this a pending item that will become live in the documentation?

@hflynn
Copy link
Member Author

hflynn commented Aug 8, 2013

The commented block is because the online validator isn't working at the moment and the link redirects to the FAQ text I have reproduced here for the BF CL instructions - Andrew has a ticket to fix it so it can be live again once that is sorted.

@hflynn
Copy link
Member Author

hflynn commented Aug 8, 2013

Have added the supporter logos in one block to minimise new images - if it looks really pants, I can add them all individually.

@qidane
Copy link
Contributor

qidane commented Aug 9, 2013

"The OME-XML metadata block may contain anything a standard OME-XML file can, including multiple OME images with multiple sets of pixels." We did away with a single image having "multiple sets of pixels" as a concept a few years ago.

@qidane
Copy link
Contributor

qidane commented Aug 9, 2013

omez is out of date.

Loose the line "Lastly, we saw no advantage to zipping or 7-zipping OME-XML. If you want to distribute an OME-XML file, we recommend gzipped OME-XML (omez) format, as it is the OME standard." from ome-tiff/data.html

And remove the "(omez)" from the "OME-XML, gzipped (omez)" table row.

Also remove from "Also good are gzipped OME-XML (omez) and zipped OME-TIFF."

@qidane
Copy link
Contributor

qidane commented Aug 9, 2013

@hflynn Apart from the above comments, that are not directly related to your changes, I think this reads well and is clearer.

@hflynn
Copy link
Member Author

hflynn commented Aug 9, 2013

Realised I'd deleted an entire sentence rather than just the 'omez' for one bit, should be ok now.

@qidane
Copy link
Contributor

qidane commented Aug 9, 2013

Looks great, thanks.

@joshmoore
Copy link
Member

@hflynn : this is waiting on a new logos concept, no? (I'd vote for push forcing away the unused PNG)

@hflynn
Copy link
Member Author

hflynn commented Aug 13, 2013

Logos removed, anything else?

@hflynn
Copy link
Member Author

hflynn commented Aug 14, 2013

@ctrueden any comments before we merge & release this?

@joshmoore If there are no further comments, please merge Thursday first thing.

@ctrueden
Copy link
Member

@hflynn: Thanks for this work. Some comments follow. Note that none of these are blockers to merging this PR. I just wanted to write my thoughts somewhere and this seemed as good a place as any.

  1. We are now really heavily deemphasizing OME-XML as a binary format. This makes sense; we have been informally doing that for years. So perhaps we should also change the OME-XML page to mention that OME-TIFF is the preferred container format? Perhaps a section at the top in a "note" box?

  2. I think the OME-TIFF sample data page should link to the OME-XML page, since it compares OME-TIFF and OME-XML binary formats (and various means to compress them), but does not actually explain what OME-XML is there.

  3. I still miss the color coding that the Best/Great/Good/Poor/Worst table originally had before it was migrated. IMHO, it made it much easier to absorb the differences at a glance. We should be able to do that in Sphinx...

  4. This may be more of a comment for @joshmoore or @sbesson, but I noticed that links to code now link into the snoopycrimecop fork instead of the openmicroscopy codebase. I am sure there are technical reasons for it, but it still strikes me as very confusing and largely defeating the purpose of the code link (no one outside the project will know where the code actually lives from such links).

  5. On the OME-TIFF example source code page, some code snippets are out of date. To match the linked source code, the snippet TiffParser.getComment(f) should now read new TiffParser(f).getComment();, and TiffSaver.overwriteComment(f, xml) should now read:

TiffSaver saver = new TiffSaver(f);
RandomAccessInputStream in = new RandomAccessInputStream(f);
saver.overwriteComment(in, xml);
  1. Also on the OME-TIFF example source code page, the link to the Bio-Formats library is:
http://www.openmicroscopy.org/sitesite/products/ome5/bio-formats/

Should that be one of:

http://www.openmicroscopy.org/site/products/bio-formats/
http://www.openmicroscopy.org/site/products/ome5/bio-formats/

?

  1. Also on the OME-TIFF example source code page, it is mentioned that an OMEXMLMetadata is "at heart an XML DOM object in memory" but this is no longer true; @chris-allan and @melissalinkert refactored it to be a true Java object hierarchy.

  2. Also on that page, there is a link to the "Bio-Formats tools" which should be "Bio-Formats command line tools" (to avoid ambiguity). It also links to the OME5-specific page, rather than the general page (maybe that is correct; I don't know).

Again, these are all not intended to block merging this specific PR—just general documentation suggestions spurred by the fact that @hflynn asked for my review.

@sbesson
Copy link
Member

sbesson commented Aug 14, 2013

Answering 4), the snoopycrimecop remote is used for PR review solely, specifically to allow simultaneous code/documentation review.
Once a PR is merged in, the production docs are built with all source code links pointing at the openmicroscopy repository.

@ctrueden
Copy link
Member

@sbesson: Thanks for the clarification, that is neat.

@sbesson
Copy link
Member

sbesson commented Aug 14, 2013

No problem, I leave the rest of your comments to @hflynn :)

@hflynn
Copy link
Member Author

hflynn commented Aug 15, 2013

Thanks very much @ctrueden - those are really helpful and aside from the table thing, which I will have a play with another time, easily fixed here.

@hflynn
Copy link
Member Author

hflynn commented Aug 15, 2013

@ctrueden Just to explain BTW - this is on the develop branch and is the documentation for the June 2013 data model schema so it links to all OME 5 pages because the latest version is not supported by 4.4.

@qidane
Copy link
Contributor

qidane commented Aug 15, 2013

@hflynn Looks good to merge.

joshmoore added a commit that referenced this pull request Aug 15, 2013
Data Model & OME-TIFF Docs rework
@joshmoore joshmoore merged commit 98f081e into ome:develop Aug 15, 2013
@hflynn hflynn deleted the tiff_rework branch August 15, 2013 12:39
@jburel
Copy link
Member

jburel commented Sep 9, 2013

--no-rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants