Implements collections of bundles. by cmnbroad · Pull Request #1702 · samtools/htsjdk

cmnbroad · 2024-01-23T21:15:40Z

Also contains some commits with minor updates to take advantage of Java 17 and Java text blocks.

lbergelson

A few little issues.

I recommend we change from returning bundle Collection to bundle List. It's rare we actually want an unsorted bag of file names.

lbergelson · 2024-01-24T16:14:07Z

src/main/java/htsjdk/beta/io/bundle/Bundle.java

    private static final long serialVersionUID = 1L;

-    private final Map<String, BundleResource> resources = new LinkedHashMap<>();
+    // don't use  LinkedHashMap here; using HashMap resolves unnatural resource ordering issues that arise


I'm not sure I follow what's going on here when you have a LinkedHashMap. Shouldn't serializing a linked hashmap deserialize it to the same map?

Hm. I was having problems when roundtripping these through JSON, but I can no longer reproduce the issue, so reverting to our beloved LinkedHashMap.

Long Live LinkedHashMap!

So I realized what the issue was here. JSONObject internally uses a HashMap (implying, I think, that JSON doesn't preserve the serialized order of JSON attributes), so when you roundtrip through JSON, the iteration order from JSON is based on the HashMap order. If we use LinkedHashMap in Bundle, then the order after a roundtrip gets scrambled, and tests fail (but only for some cases because sometimes the roundtrip order matches and sometimes it differs).

Ahah, that makes sense. Seems like a weird decision to not maintain internal order, but it's good to know about. Cana we change our tests to use an order independent comparison?

I added an resource-order-independent equals method for use in the tests.

lbergelson · 2024-01-24T17:49:54Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java


    final private static Set<String> TOP_LEVEL_PROPERTIES = Collections.unmodifiableSet(
-            new HashSet<String>() {
+            new HashSet<>() {


Can these static declarations be migrated to use Set.of() instead of these anonymous classes?

Oh yeah, thats an improvement. Done.

lbergelson · 2024-01-24T17:56:42Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java

+                final Collection<Bundle> bundles = toBundleCollection(jsonString, ioPathConstructor);
+                if (bundles.size() > 1) {
+                    throw new IllegalArgumentException(
+                        String.format("A JSON string with more than one bundle was provided but only a single Bundle is allowed",


The format string is missing the template variables.

lbergelson · 2024-01-24T18:03:12Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java

+                        e);
            }
+        }
+        if (bundles.size() < 1) {


Nitpicky but I isEmpty might be better

lbergelson · 2024-01-24T18:06:53Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java

+            final String primaryContentType = getRequiredPropertyAsString(jsonObject, JSON_PROPERTY_PRIMARY);
+            final Collection<BundleResource> bundleResources = toBundleResources(jsonObject, ioPathConstructor);
+            if (bundleResources.isEmpty()) {
+                LOG.warn("Empty resource bundle found in: ", jsonObject.toString());


Should we allow empty bundles at all?

Actually, we already don't. I'll remove this (the Bundle constructor will throw anyway), and add a negative test to BundleTest to demonstrate that.

lbergelson · 2024-01-24T18:07:24Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java

+        return new IOPathResource(
+                ioPathConstructor.apply(getRequiredPropertyAsString(jsonObject, JSON_PROPERTY_PATH)),
+                contentType,
+                format == null ? null : format);


I think this ternary is unnecessary since you're not dereferencing format here.

lbergelson · 2024-01-24T18:09:00Z

src/test/java/htsjdk/beta/io/bundle/BundleJSONTest.java

-                                "}\n",
-                        BundleResourceType.ALIGNED_READS,
-                        Arrays.asList(BundleResourceTestData.readsWithFormat)
+                    """


This is SO much nicer.

lbergelson · 2024-01-24T18:09:49Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java

    public static final String JSON_PROPERTY_FORMAT           = "format";
+
    public static final String JSON_SCHEMA_NAME               = "htsbundle";
    public static final String JSON_SCHEMA_VERSION            = "0.1.0"; // TODO: bump this to 1.0.0


Should this be 0.2.0 now?

Will update the schema in a separate PR when I move the bundle classes out of the beta package.

lbergelson · 2024-01-24T18:15:58Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java

+     * @return Collection<Bundle>
+     * @param <T> IOPath-derived class to use for IOPathResources
+     */
+    public static <T extends IOPath> Collection<Bundle> toBundleCollection(


Do we really want this to be Collection and not something with an order? I would think List might be better? The input order is usually important in some way, changing the encounter order can often change either errors generated or floating point aggregations even if it's something that seems like it should be file order agnostic.

Changed to Lists.

cmnbroad · 2024-06-11T18:15:58Z

@lbergelson I updated the names and organization of the bundle content type strings in this, which is why there are now so many changes.

lbergelson

@cmnbroad Looks good to me. I'm not 100% sure why you want to decouple the content type names from the enums but that seems fine.

cmnbroad added 3 commits January 15, 2024 15:44

Update to Java text blocks for embedded JSON strings.

196c169

Don't use LinkedHashMap for bundles, update comments.

3df0b8b

Implement collections of bundles.

3f0d79d

cmnbroad mentioned this pull request Jan 23, 2024

Implement VCF bundles. #1703

Merged

lbergelson requested changes Jan 24, 2024

View reviewed changes

cmnbroad force-pushed the cn_bundle_updates branch 2 times, most recently from 437912b to 1be2b85 Compare April 8, 2024 21:02

cmnbroad added 2 commits May 29, 2024 11:07

Review comments.

aa44a43

Fix HtsPath equals and hashCode.

8b0e589

cmnbroad force-pushed the cn_bundle_updates branch from 1be2b85 to 8b0e589 Compare May 29, 2024 15:09

cmnbroad added 3 commits June 10, 2024 17:11

Update bundle content type names.

99cd8c2

Update ReadsBundleTest to use more defined constants.

36764e1

Parameterize BundleJSONTest cases.

79ba8d9

cmnbroad force-pushed the cn_bundle_updates branch from bf33fee to 79ba8d9 Compare June 11, 2024 14:45

Don't use type names as external content type strings.

e6cfc62

This was referenced Aug 6, 2024

Bundle support for FeatureInputs, references; add CreateBundle tool. broadinstitute/gatk#8944

Draft

Support reference bundles. #1713

Merged

lbergelson approved these changes Aug 13, 2024

View reviewed changes

cmnbroad merged commit 204a0db into master Aug 13, 2024

cmnbroad deleted the cn_bundle_updates branch August 13, 2024 22:00

Conversation

cmnbroad commented Jan 23, 2024

Uh oh!

lbergelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmnbroad Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmnbroad commented Jun 11, 2024

Uh oh!

lbergelson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cmnbroad Mar 12, 2024 •

edited

Loading